[BEAM-1928] Add SdkComponents by tgroh · Pull Request #2459 · apache/beam

tgroh · 2017-04-07T16:35:37Z

Be sure to do all of the following to help us incorporate your contribution
quickly and easily:

Make sure the PR title is formatted like:
[BEAM-<Jira issue #>] Description of pull request
Make sure tests pass via mvn clean verify. (Even better, enable
Travis-CI on your fork and ensure the whole test matrix passes).
Replace <Jira issue #> in the title with the actual Jira issue
number, if there is one.
If this contribution is large, please file an Apache
Individual Contributor License Agreement.

This takes SDK objects and assigns IDs to them. It is effectively a
ComponentsBuilder context where a component is referred to by the
Java object which is being translated, rather than by an opaque
string or protocol buffer.

tgroh · 2017-04-07T16:35:43Z

R: @kennknowles

coveralls · 2017-04-07T17:43:59Z

Coverage increased (+0.06%) to 69.86% when pulling b55849d on tgroh:sdk_components into e0df458 on apache:master.

asfbot · 2017-04-07T17:45:49Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/9283/

Failed Tests: 1

beam_PreCommit_Java_MavenInstall/org.apache.beam:beam-sdks-java-core: 1

org.apache.beam.sdk.util.SdkComponentsTest.putPCollectionExistingNameCollision

--none--

coveralls · 2017-04-07T17:48:09Z

Coverage increased (+0.05%) to 69.854% when pulling b55849d on tgroh:sdk_components into e0df458 on apache:master.

asfbot · 2017-04-07T17:53:13Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/9285/
--none--

asfbot · 2017-04-07T18:46:31Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/9287/
--none--

asfbot · 2017-04-07T19:52:36Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/9289/
--none--

asfbot · 2017-04-07T19:56:06Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/9290/
--none--

asfbot · 2017-04-07T20:49:19Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/9292/
--none--

This takes SDK objects and assigns IDs to them. It is effectively a ComponentsBuilder context where a component is referred to by the Java object which is being translated, rather than by an opaque string or protocol buffer. The Builder for SdkComponents exposes mutation methods to add components. The actual SdkComponents implementation is deeply immutable, to signal that during actual conversion to/from protos should assume that all elements are already recorded as a component.

coveralls · 2017-04-07T21:56:49Z

Coverage increased (+0.04%) to 69.846% when pulling c00d8b3 on tgroh:sdk_components into 82c5e89 on apache:master.

asfbot · 2017-04-07T21:58:19Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/9294/
--none--

tgroh · 2017-04-07T23:46:40Z

R: @lukecwik

tgroh · 2017-04-07T23:47:58Z

For reference, I have a branch https://github.com/tgroh/beam/tree/coders_utilities that is approximately where I expect this to go, after implementing each type of component translation.

lukecwik · 2017-04-10T16:54:36Z

sdks/java/core/src/main/java/org/apache/beam/sdk/util/SdkComponents.java

+   * <p>Uses the full name of the transform as the id.
+   */
+  public String getTransformId(AppliedPTransform<?, ?, ?> pTransform) {
+    String existing = transformIds.get(pTransform);


I spoke with Ken and he gave me the impression that that the id in the components maps was some kind of UUID and not equivalent or related to the unique name.

@kennknowles Comment?

It is independent, but a fine choice. The "id" is just a pointer into the map and has no other significance. The unique name is required for e.g. identifying state associated with the transform across pipeline updates. (and whatever other programmatic uses a runner might have for a unique name)

This is easy to change to be unrelated. Done.

The reason I did this was a failure to realize the independence of unique_name and the keys in the components.

I would stick with the longer readable name that you had, its easy for a runner to walk the pipeline and produce a version of the pipeline with short names if it thinks that it is valuable.

Done. Reverted.

lukecwik · 2017-04-10T18:09:05Z

sdks/java/core/src/main/java/org/apache/beam/sdk/util/SdkComponents.java

+   * <p>Uses the full name of the transform as the id.
+   */
+  public String getTransformId(AppliedPTransform<?, ?, ?> pTransform) {
+    String existing = transformIds.get(pTransform);


This assumes that the user never overrides equals when creating a subclass of pTransform.

Is instance equality all you need or more? (Consider using Equivalence.Wrapper)

Applied Transforms are only equal if they have identical names, among other things. Given that names must be unique in the graph, this is sufficient to ensure repeated applications show up multiple times.

lukecwik · 2017-04-10T18:09:44Z

sdks/java/core/src/main/java/org/apache/beam/sdk/util/SdkComponents.java

+   * <p>Uses a unique name based on the result of {@link PCollection#getName()}.
+   */
+  public String getPCollectionId(PCollection<?> pCollection) {
+    String existing = pCollectionIds.get(pCollection);


Make PCollection final to prevent overriding and messing up the equals method or use an equivalence wrapper?

It's impossible to construct a PCollection subclass, as it has no visible constructors.

lukecwik · 2017-04-10T18:10:29Z

sdks/java/core/src/main/java/org/apache/beam/sdk/util/SdkComponents.java

+   * SdkComponents}.
+   */
+  public String getWindowingStrategyId(WindowingStrategy<?, ?> windowingStrategy) {
+    String existing = windowingStrategyIds.get(windowingStrategy);


Make windowing strategy final or use an equivalence wrapper?

lukecwik · 2017-04-10T18:16:53Z

sdks/java/core/src/main/java/org/apache/beam/sdk/util/SdkComponents.java

+   * <p>Uses the full name of the transform as the id.
+   */
+  public String getTransformId(AppliedPTransform<?, ?, ?> pTransform) {
+    String existing = transformIds.get(pTransform);


I would stick with the longer readable name that you had, its easy for a runner to walk the pipeline and produce a version of the pipeline with short names if it thinks that it is valuable.

lukecwik · 2017-04-10T18:17:59Z

sdks/java/core/src/main/java/org/apache/beam/sdk/util/SdkComponents.java

+   * returning the id of the {@link Coder} in this {@link SdkComponents}.
+   */
+  public String getCoderId(Coder<?> coder) {
+    String existing = coderIds.get(coder);


Equivalence wrapper?

lukecwik · 2017-04-10T18:22:21Z

sdks/java/core/src/main/java/org/apache/beam/sdk/util/SdkComponents.java

+   *
+   * <p>Uses the full name of the transform as the id.
+   */
+  public String getTransformId(AppliedPTransform<?, ?, ?> pTransform) {


registerPTransform

Update comment to something along:
registers a ptransform assigning a unique id, multiple registrations of the same ptransform return the id of the original transform.

Ditto for the rest like:
registerCoder
registerWindowingStrategy

+1 specifically because you are building an object that you will convert to proto, hence the set of all registered things is observable.

Phrasing on the spec: multiple registration of the same transform yield the same id

lukecwik · 2017-04-10T18:24:03Z

sdks/java/core/src/main/java/org/apache/beam/sdk/util/SdkComponents.java

+   * PCollection PCollections}, and {@link PTransform PTransforms}.
+   */
+  @Experimental
+  public RunnerApi.Components toComponents() {


Would it better be to just implement a PipelineVisitor that converts from a "java" pipeline to the proto representation, this way you don't need to expose public methods and are effectively walking the graph?

~ish. We should not use these from outside of this package (SomeComponent.toProto should use this as the underlying way to construct a MessageWithComponents), but we want to be able to convert arbitrary components without the entire pipeline.

coveralls · 2017-04-10T19:09:05Z

Coverage increased (+0.05%) to 70.211% when pulling c00d8b3 on tgroh:sdk_components into 80d2548 on apache:master.

asfbot · 2017-04-10T19:10:29Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/9358/

Failed Tests: 1

beam_PreCommit_Java_MavenInstall/org.apache.beam:beam-sdks-java-core: 1

org.apache.beam.sdk.util.SdkComponentsTest.getTransformId

--none--

coveralls · 2017-04-10T19:27:35Z

Coverage increased (+0.04%) to 70.21% when pulling c00d8b3 on tgroh:sdk_components into 80d2548 on apache:master.

asfbot · 2017-04-10T19:29:09Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/9360/
--none--

tgroh

I've also made this class mostly package private, and moved it to runners-core-construction so the Protos aren't exposed to users.

tgroh · 2017-04-10T18:37:12Z

sdks/java/core/src/main/java/org/apache/beam/sdk/util/SdkComponents.java

+   * <p>Uses a unique name based on the result of {@link PCollection#getName()}.
+   */
+  public String getPCollectionId(PCollection<?> pCollection) {
+    String existing = pCollectionIds.get(pCollection);


It's impossible to construct a PCollection subclass, as it has no visible constructors.

tgroh · 2017-04-10T18:37:41Z

sdks/java/core/src/main/java/org/apache/beam/sdk/util/SdkComponents.java

+   * SdkComponents}.
+   */
+  public String getWindowingStrategyId(WindowingStrategy<?, ?> windowingStrategy) {
+    String existing = windowingStrategyIds.get(windowingStrategy);


tgroh · 2017-04-10T18:41:10Z

sdks/java/core/src/main/java/org/apache/beam/sdk/util/SdkComponents.java

+   * PCollection PCollections}, and {@link PTransform PTransforms}.
+   */
+  @Experimental
+  public RunnerApi.Components toComponents() {


~ish. We should not use these from outside of this package (SomeComponent.toProto should use this as the underlying way to construct a MessageWithComponents), but we want to be able to convert arbitrary components without the entire pipeline.

tgroh · 2017-04-10T20:09:19Z

sdks/java/core/src/main/java/org/apache/beam/sdk/util/SdkComponents.java

+   * <p>Uses the full name of the transform as the id.
+   */
+  public String getTransformId(AppliedPTransform<?, ?, ?> pTransform) {
+    String existing = transformIds.get(pTransform);


Applied Transforms are only equal if they have identical names, among other things. Given that names must be unique in the graph, this is sufficient to ensure repeated applications show up multiple times.

tgroh · 2017-04-10T20:09:26Z

sdks/java/core/src/main/java/org/apache/beam/sdk/util/SdkComponents.java

+   * <p>Uses the full name of the transform as the id.
+   */
+  public String getTransformId(AppliedPTransform<?, ?, ?> pTransform) {
+    String existing = transformIds.get(pTransform);


Done. Reverted.

tgroh · 2017-04-10T20:15:08Z

sdks/java/core/src/main/java/org/apache/beam/sdk/util/SdkComponents.java

+   *
+   * <p>Uses the full name of the transform as the id.
+   */
+  public String getTransformId(AppliedPTransform<?, ?, ?> pTransform) {


tgroh · 2017-04-10T20:15:19Z

sdks/java/core/src/main/java/org/apache/beam/sdk/util/SdkComponents.java

+   * returning the id of the {@link Coder} in this {@link SdkComponents}.
+   */
+  public String getCoderId(Coder<?> coder) {
+    String existing = coderIds.get(coder);


asfbot · 2017-04-10T20:29:14Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/9365/

Build result: FAILURE

[...truncated 808.64 KB...] at hudson.remoting.UserRequest.perform(UserRequest.java:153) at hudson.remoting.UserRequest.perform(UserRequest.java:50) at hudson.remoting.Request$2.run(Request.java:336) at hudson.remoting.InterceptingExecutorService$1.call(InterceptingExecutorService.java:68) at java.util.concurrent.FutureTask.run(FutureTask.java:266) at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142) at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617) at java.lang.Thread.run(Thread.java:745)Caused by: org.apache.maven.plugin.MojoExecutionException: Dependency problems found at org.apache.maven.plugins.dependency.analyze.AbstractAnalyzeMojo.execute(AbstractAnalyzeMojo.java:261) at org.apache.maven.plugin.DefaultBuildPluginManager.executeMojo(DefaultBuildPluginManager.java:134) at org.apache.maven.lifecycle.internal.MojoExecutor.execute(MojoExecutor.java:208) ... 31 more2017-04-10T20:29:02.128 [ERROR] 2017-04-10T20:29:02.128 [ERROR] Re-run Maven using the -X switch to enable full debug logging.2017-04-10T20:29:02.128 [ERROR] 2017-04-10T20:29:02.128 [ERROR] For more information about the errors and possible solutions, please read the following articles:2017-04-10T20:29:02.129 [ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoExecutionException2017-04-10T20:29:02.129 [ERROR] 2017-04-10T20:29:02.129 [ERROR] After correcting the problems, you can resume the build with the command2017-04-10T20:29:02.130 [ERROR] mvn -rf :beam-runners-core-construction-javachannel stoppedSetting status of e46df61 to FAILURE with url https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/9365/ and message: 'Build finished. 'Using context: Jenkins: Maven clean install
--none--

lukecwik

Minor comments and then LGTM

lukecwik · 2017-04-10T20:29:17Z

...construction-java/src/main/java/org/apache/beam/runners/core/construction/SdkComponents.java

-   * present, returning the id of the provided {@link PCollection} in this {@link SdkComponents}.
+   * Registers the provided {@link PCollection} into this {@link SdkComponents}, returning a unique
+   * ID for the {@link PCollection}. Multiple calls to {@link #registerPCollection(PCollection)}
+   * with the same {@link PCollection}will return the same unique ID.


missing space
{@link PCollection}will -> {@link PCollection} will

lukecwik · 2017-04-10T20:30:19Z

...construction-java/src/main/java/org/apache/beam/runners/core/construction/SdkComponents.java

+   * ID for the {@link PCollection}. Multiple calls to {@link #registerPCollection(PCollection)}
+   * with the same {@link PCollection}will return the same unique ID.
   *
   * <p>Uses a unique name based on the result of {@link PCollection#getName()}.


I would drop the bit about uses a unique name, since these are internal details. For all intensive purposes your returning random ids.

lukecwik · 2017-04-10T20:31:25Z

...construction-java/src/main/java/org/apache/beam/runners/core/construction/SdkComponents.java

-   * SdkComponents}.
+   * Registers the provided {@link WindowingStrategy} into this {@link SdkComponents}, returning a
+   * unique ID for the {@link WindowingStrategy}. Multiple calls to {@link
+   * #registerWindowingStrategy(WindowingStrategy)} with the equal {@link WindowingStrategy


with the equal -> with equal
WindowingStrategies -> windowing strategies

lukecwik · 2017-04-10T20:32:01Z

...construction-java/src/main/java/org/apache/beam/runners/core/construction/SdkComponents.java

-   * Puts the provided {@link Coder} into this {@link SdkComponents} if it is not already present,
-   * returning the id of the {@link Coder} in this {@link SdkComponents}.
+   * Registers the provided {@link Coder} into this {@link SdkComponents}, returning a unique ID for
+   * the {@link Coder}. Multiple calls to {@link #registerCoder(Coder)} with the identical {@link


with the identical -> with identical

tgroh · 2017-04-10T21:06:22Z

Method names updated everywhere, comments updated everywhere.

lukecwik

LGTM

coveralls · 2017-04-10T22:09:13Z

Coverage increased (+0.003%) to 70.169% when pulling 6f8175f on tgroh:sdk_components into 80d2548 on apache:master.

asfbot · 2017-04-10T22:13:16Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/9368/
--none--

tgroh force-pushed the sdk_components branch from 3796047 to b55849d Compare April 7, 2017 16:55

tgroh force-pushed the sdk_components branch from b55849d to b2f02bd Compare April 7, 2017 18:35

tgroh force-pushed the sdk_components branch 2 times, most recently from b371783 to 0783c3e Compare April 7, 2017 19:49

tgroh force-pushed the sdk_components branch from 0783c3e to f15282b Compare April 7, 2017 20:43

tgroh force-pushed the sdk_components branch from f15282b to c00d8b3 Compare April 7, 2017 20:53

lukecwik reviewed Apr 10, 2017

View reviewed changes

tgroh force-pushed the sdk_components branch from 8d12ac4 to c00d8b3 Compare April 10, 2017 18:32

tgroh added 2 commits April 10, 2017 13:16

fixup! Add SdkComponents

ab7ec6f

fixup! Add SdkComponents

e46df61

tgroh commented Apr 10, 2017

View reviewed changes

lukecwik requested changes Apr 10, 2017

View reviewed changes

fixup! Add SdkComponents

6f8175f

lukecwik approved these changes Apr 10, 2017

View reviewed changes

tgroh changed the title ~~Add SdkComponents~~ [BEAM-1928] Add SdkComponents Apr 10, 2017

asfgit closed this in 8afa623 Apr 10, 2017

Comments

Conversation

tgroh commented Apr 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tgroh commented Apr 7, 2017

Uh oh!

coveralls commented Apr 7, 2017

Uh oh!

asfbot commented Apr 7, 2017

Failed Tests: 1

beam_PreCommit_Java_MavenInstall/org.apache.beam:beam-sdks-java-core: 1

Uh oh!

coveralls commented Apr 7, 2017

Uh oh!

asfbot commented Apr 7, 2017

Uh oh!

asfbot commented Apr 7, 2017

Uh oh!

asfbot commented Apr 7, 2017

Uh oh!

asfbot commented Apr 7, 2017

Uh oh!

asfbot commented Apr 7, 2017

Uh oh!

coveralls commented Apr 7, 2017

Uh oh!

asfbot commented Apr 7, 2017

Uh oh!

tgroh commented Apr 7, 2017

Uh oh!

tgroh commented Apr 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Apr 10, 2017

Uh oh!

asfbot commented Apr 10, 2017

Failed Tests: 1

beam_PreCommit_Java_MavenInstall/org.apache.beam:beam-sdks-java-core: 1

Uh oh!

coveralls commented Apr 10, 2017

Uh oh!

asfbot commented Apr 10, 2017

tgroh commented Apr 7, 2017 •

edited

Loading