Skip to content

[BEAM-11254] New documentation on multi-language pipelines - #13317

Merged
chamikaramj merged 6 commits into
apache:masterfrom
davidwrede:multi-language-pipelines
Nov 20, 2020
Merged

[BEAM-11254] New documentation on multi-language pipelines#13317
chamikaramj merged 6 commits into
apache:masterfrom
davidwrede:multi-language-pipelines

Conversation

@davidwrede

@davidwrededavidwrede commented Nov 12, 2020

Copy link
Copy Markdown
Contributor

Documentation on how to create and consume cross-language transforms in a multi-language pipeline.


Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

  • Choose reviewer(s) and mention them in a comment (R: @username).
  • Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
  • Update CHANGES.md with noteworthy changes.
  • If this contribution is large, please file an Apache Individual Contributor License Agreement.

See the Contributor Guide for more tips on how to make review process smoother.

Post-Commit Tests Status (on master branch)

LangSDKDataflowFlinkSamzaSparkTwister2
GoBuild Status---Build Status---Build Status---
JavaBuild StatusBuild Status
Build Status
Build Status
Build Status
Build Status
Build Status
Build StatusBuild Status
Build Status
Build Status
Build Status
PythonBuild Status
Build Status
Build Status
Build Status
Build Status
Build Status
Build Status
Build Status
---Build Status---
XLangBuild Status---Build Status---Build Status---

Pre-Commit Tests Status (on master branch)

---JavaPythonGoWebsiteWhitespaceTypescript
Non-portableBuild StatusBuild Status
Build Status
Build Status
Build Status
Build StatusBuild StatusBuild StatusBuild Status
Portable---Build Status------------

See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.

GitHub Actions Tests Status (on master branch)

Build python source distribution and wheels
Python tests
Java tests

See CI.md for more information about GitHub Actions CI.

@chamikaramjchamikaramj changed the title New documentation on multi-language pipelines[BEAM-11254] New documentation on multi-language pipelinesNov 12, 2020
Comment threadwebsite/www/site/content/en/documentation/programming-guide.md Outdated

@chamikaramjchamikaramj left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Dave.

I think this is in a great state. My comments are mostly nits.

Also seems like there are some test failures that have to be addressed.

Comment threadwebsite/www/site/content/en/documentation/programming-guide.md Outdated
Comment threadwebsite/www/site/content/en/documentation/programming-guide.md Outdated
Comment threadwebsite/www/site/content/en/documentation/programming-guide.md Outdated
Comment threadwebsite/www/site/content/en/documentation/programming-guide.md Outdated
Comment threadwebsite/www/site/content/en/documentation/programming-guide.md Outdated
Comment threadwebsite/www/site/content/en/documentation/programming-guide.md Outdated
Comment threadwebsite/www/site/content/en/documentation/programming-guide.md Outdated
Comment threadwebsite/www/site/content/en/documentation/programming-guide.md Outdated
Comment threadwebsite/www/site/content/en/documentation/programming-guide.md Outdated
Comment threadwebsite/www/site/content/en/documentation/programming-guide.md Outdated

@robertwbrobertwb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, it'll be great to have this documented!

Comment threadwebsite/www/site/content/en/documentation/programming-guide.md Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the double cast needed? Perhaps it would be simpler to add the type parameters to ImmutableMap.of, i.e. ImmutableMap.of<String, Class<? extends ExternalTransformBuilder<?, ?, ?>>>(...).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I tried the suggestion and it doesn't seem to be working. Note that this snippet is from actual class: https://github.com/apache/beam/blob/master/sdks/java/io/kafka/src/main/java/org/apache/beam/sdk/io/kafka/KafkaIO.java#L632

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, it's a bit ugly we can leave it as is for now.

Comment threadwebsite/www/site/content/en/documentation/programming-guide.md Outdated
Comment threadwebsite/www/site/content/en/documentation/programming-guide.md Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not setTopics(config.topics) or, if a copy is needed, setTopics(ImmutableList.copyOf(config.topics))

(FWIW, the motivation to have a separate Builder and Configuration object that seem to largely play the same role is a bit unclear here--it'd be good to clarify that.)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Comment threadwebsite/www/site/content/en/documentation/programming-guide.md Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that this will only serve those transforms living in modules that have been imported...

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a point to import modules.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do you specify the custom container?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we have fully implemented dependencies support for Python external transforms yet so we are limited to dependencies available with core Beam SDK Harness. Updated text to reflect this.

Comment threadwebsite/www/site/content/en/documentation/programming-guide.md Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

self.expansion_service?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

@davidwrede

davidwrede commented Nov 14, 2020

Copy link
Copy Markdown
ContributorAuthor

@robertwb I implemented some of your changes, but unfortunately, someone else will have to address the rest.

I'm happy to help merge in the PR as is, or hand it off to someone else.

@robertwb

Copy link
Copy Markdown
Contributor

Thanks, @davidwrede! We'll take it from here.

@chamikaramjchamikaramj left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Robert. PTAL.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I tried the suggestion and it doesn't seem to be working. Note that this snippet is from actual class: https://github.com/apache/beam/blob/master/sdks/java/io/kafka/src/main/java/org/apache/beam/sdk/io/kafka/KafkaIO.java#L632

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a point to import modules.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we have fully implemented dependencies support for Python external transforms yet so we are limited to dependencies available with core Beam SDK Harness. Updated text to reflect this.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have it there as well. See "Using an SDK wrapper". Primary audience of this sub-section are transforms authors. So information here is more on writing wrappers. There's some overlap but I think this is fine.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to include information about both JavaJarExpansionService and BeamJarExpansionService.

@chamikaramj

Copy link
Copy Markdown
Contributor

Also cc: @rosetn in case docs team have more comments here.

@chamikaramj
chamikaramjforce-pushed the multi-language-pipelines branch from f150e63 to a208491CompareNovember 20, 2020 00:01

@robertwbrobertwb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this looks good to me.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, it's a bit ugly we can leave it as is for now.

@chamikaramj

Copy link
Copy Markdown
Contributor

Thanks!

@chamikaramj
chamikaramj merged commit 626551e into apache:masterNov 20, 2020
@davidwrede
davidwrede deleted the multi-language-pipelines branch December 4, 2020 18:57
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@davidwrede@robertwb@chamikaramj@TheNeuralBit