[BEAM-9273] Explicitly disable @RequiresTimeSortedInput on unsupported runners by je-ik · Pull Request #10816 · apache/beam

je-ik · 2020-02-10T11:26:47Z

Fixes [BEAM-9273]

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)

Lang	SDK	Apex	Dataflow	Gearpump	Samza	Spark
Go		---	---	---	---
Java
Python		---		---	---
XLang	---	---	---	---	---	---

Pre-Commit Tests Status (on master branch)

---	Java	Python	Go	Website
Non-portable
Portable	---		---	---

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

je-ik · 2020-02-10T11:42:26Z

Run Flink ValidatesRunner

je-ik · 2020-02-10T11:42:37Z

Run Spark ValidatesRunner

je-ik · 2020-02-10T11:43:02Z

Run Spark StructuredStreaming ValidatesRunner

je-ik · 2020-02-10T11:43:18Z

Run Apex ValidatesRunner

je-ik · 2020-02-10T11:43:35Z

Run Dataflow ValidatesRunner

je-ik · 2020-02-10T11:43:42Z

Run Samza ValidatesRunner

…d runners

je-ik · 2020-02-10T18:42:15Z

Run Flink ValidatesRunner

je-ik · 2020-02-10T18:42:31Z

Run Dataflow ValidatesRunner

je-ik · 2020-02-10T18:42:52Z

Run Spark StructuredStreaming ValidatesRunner

je-ik · 2020-02-10T18:43:11Z

Run Apex ValidatesRunner

je-ik · 2020-02-10T18:43:40Z

Run Spark ValidatesRunner

je-ik · 2020-02-10T18:44:17Z

Run Samza ValidatesRunner

je-ik · 2020-02-10T18:44:35Z

Run Gearpump ValidatesRunner

je-ik · 2020-02-10T19:43:29Z

The fail seems not to be related to this PR. First failed post commit seems to be this one.

kennknowles

Meta-comment: switching the whole world to use DoFnFeatures would be best as a separate commit, since it does not have to do with RequiresTimeSortedInput. It is OK to leave as one commit since it isn't that big a deal.

kennknowles · 2020-02-11T02:18:20Z

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

+import org.apache.beam.sdk.values.TypeDescriptor;
+
+/**
+ * Features a {@link DoFn} can posses. Each runner might implement a different (sub)set of this


The DoFnSignature is precisely the list of features already, no? It is OK to have helper methods anyhow.

Style point: for a given class, utility static methods usually go in a companion class like DoFnSignature (class) & DoFnSignatures (utility methods)

Agree. The reason I created this class is that I wanted to demonstrate the approach of declaring pipeline requirements and validating that runner supports all requirements.

That would go as follows:

Set<Features> required = PipelineFeaturs.extract(pipeline); Runner.validateAllFeaturesSupported(required);

That way, adding a new feature to pipeline would not require any change to runner core, but the runner would reject the pipeline.

I then realized that this change would be too big and really not related to the annotation, so I returned back to the original approach, where runners explicitly reject features they do not support (and adding unsupported feature needs modification in runners code). I will rename the class as you suggest.

Hmmm ... DoFnSignatures already exists. It does something different (creates DoFnSignature from DoFn). Feels weird to mix these two helper functionalities.

@kennknowles would you have any suggestions about naming the class? I think this code really should not go to DoFnSignatures, can we agree on some alternative name?

I quite strongly believe it belongs in DoFnSignatures and that what you describe in the rest belongs in Pipelines. Static-method-only utility classes tend to be disorganized and undiscoverable. It is better to attach them to the thing that they are most related to.

Moved the code there. The reason I didn't like it is that DoFnSignatures probably when created served the purpose to create signatures from DoFn, while the code I added makes no use of DoFnSignature itself. But there seems to be more code like that already, so I'm fine with that.

...struction-java/src/test/java/org/apache/beam/runners/core/construction/DoFnFeaturesTest.java

kennknowles · 2020-02-11T02:21:10Z

...ataflow-java/src/main/java/org/apache/beam/runners/dataflow/BatchStatefulParDoOverrides.java

      DoFn<KV<K, InputT>, OutputT> fn = originalParDo.getFn();
      verifyFnIsStateful(fn);
-      DataflowRunner.verifyStateSupported(fn);
+      DataflowRunner.verifyDoFnSupported(fn, false);


Passing a raw bool into a function call is not very readable. I suggest splitting into verifyDoFnSupportedForStreaming and verifyDoFnSupportedForBatch. These can each call the common code.

I added both versions, although the second one verifyDoFnSupportedStreaming is not used. I added that so that the methods are not imbalanced. The streaming case is called from DataflowRunner.verifyDoFnSupported(fn, context.getPipelineOptions().isStreaming()), where it would be weird to do if (context..isStream()) verifyStreaming() else ...

...dataflow-java/src/main/java/org/apache/beam/runners/dataflow/DataflowPipelineTranslator.java

je-ik

Fixed comments from CR. I'll squash the commit after approval.

kennknowles · 2020-02-14T21:13:58Z

sdks/java/core/src/test/java/org/apache/beam/sdk/transforms/reflect/DoFnSignaturesTest.java

+
+  @Test
+  public void testAllDoFnFeatures() {
+    tests.forEach(FeatureTest::test);


Nit: there's a lot of logic in this test class. See https://testing.googleblog.com/2014/07/testing-on-toilet-dont-put-logic-in.html

It would be better to simply write out the boilerplate for each test, so each test can be read in a straight line with no context needed.

I wanted to keep the declaration of a class and its features encapsulated in single class. Agree that this doesn't play well with the rest of the DoFnSignaturesTest. But because there is effort in the direction of superseding all this with "pipeline features", I think it is fine to keep is as is for now and drop it as part of the effort later.

kennknowles

I now see that you were raising it so that the feature checks take a DoFn as an argument instead of a DoFnSignature. So, yea, the methods would then belong in DoFns.java or since it is a class you can put static methods right on DoFn. The reason this isn't great is it creates a circular dependency. So IMO it is fine as-is.

je-ik · 2020-02-14T21:46:45Z

Thanks @kennknowles

je-ik requested review from dmvk, kennknowles and reuvenlax February 10, 2020 11:26

je-ik force-pushed the BEAM-9273 branch from e791664 to 097beb2 Compare February 10, 2020 11:31

je-ik force-pushed the BEAM-9273 branch from 097beb2 to b754e17 Compare February 10, 2020 12:34

[BEAM-9273] Explicitly disable @RequiresTimeSortedInput on unsupporte…

85eeb10

…d runners

je-ik force-pushed the BEAM-9273 branch from b754e17 to 85eeb10 Compare February 10, 2020 15:53

kennknowles requested changes Feb 11, 2020

View reviewed changes

[BEAM-9273] code review - to be squashed

9e45f9d

probot-autolabeler bot added apex dataflow gearpump java jet runners samza labels Feb 12, 2020

probot-autolabeler bot added the spark label Feb 12, 2020

je-ik commented Feb 12, 2020

View reviewed changes

je-ik requested a review from kennknowles February 12, 2020 21:11

kennknowles reviewed Feb 14, 2020

View reviewed changes

kennknowles approved these changes Feb 14, 2020

View reviewed changes

je-ik merged commit a149b6b into apache:master Feb 14, 2020

je-ik deleted the BEAM-9273 branch February 14, 2020 21:40

Conversation

je-ik commented Feb 10, 2020

Fixes [BEAM-9273]

Post-Commit Tests Status (on master branch)

Pre-Commit Tests Status (on master branch)

Uh oh!

je-ik commented Feb 10, 2020

Uh oh!

je-ik commented Feb 10, 2020

Uh oh!

je-ik commented Feb 10, 2020

Uh oh!

je-ik commented Feb 10, 2020

Uh oh!

je-ik commented Feb 10, 2020

Uh oh!

je-ik commented Feb 10, 2020

Uh oh!

je-ik commented Feb 10, 2020

Uh oh!

je-ik commented Feb 10, 2020

Uh oh!

je-ik commented Feb 10, 2020

Uh oh!

je-ik commented Feb 10, 2020

Uh oh!

je-ik commented Feb 10, 2020

Uh oh!

je-ik commented Feb 10, 2020

Uh oh!

je-ik commented Feb 10, 2020

Uh oh!

je-ik commented Feb 10, 2020

Uh oh!

kennknowles left a comment

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!

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!

Uh oh!

je-ik left a comment

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!

kennknowles left a comment

Choose a reason for hiding this comment

Uh oh!

je-ik commented Feb 14, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants