[BEAM-1353] Easy PTransform style guide compliance fixes by jkff · Pull Request #1943 · apache/beam

jkff · 2017-02-08T01:24:42Z

This leaves out the more complex / potentially-controversial ones.

dhalperi · 2017-02-08T01:30:01Z

sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/Latest.java


-  private static class Globally<T> extends PTransform<PCollection<T>, PCollection<T>> {
+  /** Implementation of {@link #globally()}. */
+  public static class Globally<T> extends PTransform<PCollection<T>, PCollection<T>> {


I may have missed the relevant section in the PTransform style guide -- why is it good to expose the classes if the user need not further configure them?

In other words, why not "principle of least visibility"?

I started a thread about this on beam-dev https://lists.apache.org/thread.html/f448eb9f531bec2c0cd70e790dc8d2198d8a1bf9d2b85f94d9ac904a@%3Cdev.beam.apache.org%3E . There indeed is no section about that, but perhaps my arguments in the thread shall be convincing.

I'm not seeing how this discussion is related to that discussion.

It's more internally-evolvable to return PTransform than to return any concrete class.

And the argument about runner intercepting isn't relevant post-FnApi. No runner can inspect the Java classes specifically, only their standard serialized form.

asfbot · 2017-02-08T01:37:54Z

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

Build result: FAILURE

[...truncated 4847 lines...]Caused by: org.apache.maven.reporting.MavenReportException: Exit code: 1 - /home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_MavenInstall@2/runners/google-cloud-dataflow-java/src/main/java/org/apache/beam/runners/dataflow/DataflowRunner.java:2046: error: reference not found * {@link org.apache.beam.sdk.io.Write.Bound Write.Bound} to provide Google ^Command line was: /usr/local/asfpackages/java/jdk1.8.0_102/jre/../bin/javadoc @options @packagesRefer to the generated Javadoc files in '/home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_MavenInstall@2/runners/google-cloud-dataflow-java/target/apidocs' dir. at org.apache.maven.plugin.javadoc.AbstractJavadocMojo.executeJavadocCommandLine(AbstractJavadocMojo.java:5163) at org.apache.maven.plugin.javadoc.AbstractJavadocMojo.executeReport(AbstractJavadocMojo.java:2075) at org.apache.maven.plugin.javadoc.JavadocJar.execute(JavadocJar.java:188) ... 33 more2017-02-08T01:37:33.474 [ERROR] 2017-02-08T01:37:33.474 [ERROR] Re-run Maven using the -X switch to enable full debug logging.2017-02-08T01:37:33.474 [ERROR] 2017-02-08T01:37:33.474 [ERROR] For more information about the errors and possible solutions, please read the following articles:2017-02-08T01:37:33.474 [ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoExecutionException2017-02-08T01:37:33.475 [ERROR] 2017-02-08T01:37:33.475 [ERROR] After correcting the problems, you can resume the build with the command2017-02-08T01:37:33.475 [ERROR] mvn -rf :beam-runners-google-cloud-dataflow-javachannel stoppedSetting status of c37d550 to FAILURE with url https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/7180/ and message: 'Build finished. 'Using context: Jenkins: Maven clean install
--none--

coveralls · 2017-02-08T02:38:22Z

Coverage decreased (-0.006%) to 69.599% when pulling e4ff843 on jkff:style-guide into 8c1a577 on apache:master.

asfbot · 2017-02-08T02:38:46Z

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

jkff · 2017-02-09T00:44:35Z

I've revised my changes to the "glorified combine" transforms according to the beam-dev thread.

coveralls · 2017-02-09T01:32:21Z

Changes Unknown when pulling 3e46f94 on jkff:style-guide into ** on apache:master**.

coveralls · 2017-02-09T01:33:04Z

Changes Unknown when pulling 3e46f94 on jkff:style-guide into ** on apache:master**.

asfbot · 2017-02-09T01:33:42Z

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

asfbot · 2017-02-09T01:33:58Z

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

dhalperi · 2017-02-10T06:45:47Z

sdks/java/core/src/test/java/org/apache/beam/sdk/io/WriteTest.java

  @Test
  public void testWriteUnbounded() {
    PCollection<String> unbounded = p.apply(CountingInput.unbounded())
-        .apply(ToString.of());


We decided this one the other way. @eljefe6a

Are the other violations going to be separate PRs or part of this PR. Should we discuss the method name here or on the list?

I can just take out this commit from the PR if the rest looks fine. The list or the JIRA thread is probably the best place.

Moving this discussion to the list.

coveralls · 2017-02-28T04:28:16Z

Coverage increased (+0.001%) to 69.136% when pulling 74ab2e0 on jkff:style-guide into 16736a6 on apache:master.

asfbot · 2017-02-28T04:29:23Z

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

coveralls · 2017-02-28T23:02:37Z

Coverage decreased (-0.02%) to 69.182% when pulling a3e26dd on jkff:style-guide into a41afdc on apache:master.

asfbot · 2017-02-28T23:03:16Z

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

dhalperi

LGTM

dhalperi · 2017-02-28T23:06:35Z

sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/Latest.java

    @Override
    public TimestampedValue<T> addInput(TimestampedValue<T> accumulator,
-        TimestampedValue<T> input) {
+                                        TimestampedValue<T> input) {


optional style: usually the old indent is preferred

I usually apply google-java-format on the changed bits but in this case I guess I forgot.

dhalperi · 2017-02-28T23:06:50Z

sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/Latest.java

-        Coder<TimestampedValue<T>> inputCoder) throws CannotProvideCoderException {
+    public Coder<TimestampedValue<T>> getAccumulatorCoder(
+        CoderRegistry registry, Coder<TimestampedValue<T>> inputCoder)
+        throws CannotProvideCoderException {


optional style: usually the throws clause is further indented (I believe)

I looked at some other instances of a throws on a new line in the codebase, and this style seems more common. It is also what google-java-format produces.

dhalperi · 2017-02-28T23:06:58Z

sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/Latest.java

-          "inputCoder must be a TimestampedValueCoder, but was %s", inputCoder);
+    public Coder<T> getDefaultOutputCoder(
+        CoderRegistry registry, Coder<TimestampedValue<T>> inputCoder)
+        throws CannotProvideCoderException {


(similar throws clause)

dhalperi · 2017-02-28T23:09:00Z

sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/Sample.java

-   */
-  public static class SampleAny<T> extends PTransform<PCollection<T>, PCollection<T>> {
+  /** Implementation of {@link #any(long)}; do not use directly. */
+  @VisibleForTesting


It looks like this test is Sample-dependent only for convenience. Is it worth changing the test to avoid the need to make this public?

dhalperi · 2017-02-28T23:11:03Z

sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/ToString.java

   */
-  public static PTransform<PCollection<?>, PCollection<String>> elements() {
-    return new SimpleToString();
+  public static Elements elements() {


I'm clearly dumb, because I've asked this question in different flavors a few times, but once more for good measure. Sorry!

The last few commits in this PR left inner transforms generic; why is Elements public? Is it for further customization, or another reason for a difference in visibility?

No reason. I thought all of the public to private changes were being done in a different PR and all at once.

jkff

Also (github won't let me reply on original comments for some reason):

Removed the VisibleForTesting thing in Sample.Any
Made ToString classes back private (forgot to do it last time after the discussion on the thread)

Should be good to go after jenkins is green.

jkff · 2017-02-28T23:19:14Z

sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/Latest.java

    @Override
    public TimestampedValue<T> addInput(TimestampedValue<T> accumulator,
-        TimestampedValue<T> input) {
+                                        TimestampedValue<T> input) {


I usually apply google-java-format on the changed bits but in this case I guess I forgot.

jkff · 2017-02-28T23:23:01Z

sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/Latest.java

-        Coder<TimestampedValue<T>> inputCoder) throws CannotProvideCoderException {
+    public Coder<TimestampedValue<T>> getAccumulatorCoder(
+        CoderRegistry registry, Coder<TimestampedValue<T>> inputCoder)
+        throws CannotProvideCoderException {


I looked at some other instances of a throws on a new line in the codebase, and this style seems more common. It is also what google-java-format produces.

jkff · 2017-02-28T23:23:05Z

sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/Latest.java

-          "inputCoder must be a TimestampedValueCoder, but was %s", inputCoder);
+    public Coder<T> getDefaultOutputCoder(
+        CoderRegistry registry, Coder<TimestampedValue<T>> inputCoder)
+        throws CannotProvideCoderException {


dhalperi · 2017-02-28T23:53:02Z

Thanks Eugene. I will merge when green.

asfbot · 2017-03-01T00:13:33Z

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

Build result: FAILURE

[...truncated 1.64 MB...] 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.compiler.CompilationFailureException: Compilation failure/home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_MavenInstall@2/runners/spark/src/test/java/org/apache/beam/runners/spark/translation/streaming/CreateStreamTest.java:[117,71] cannot find symbol symbol: method withoutDefaults() location: class org.apache.beam.sdk.transforms.PTransform<org.apache.beam.sdk.values.PCollection<java.lang.Integer>,org.apache.beam.sdk.values.PCollection<java.lang.Long>> at org.apache.maven.plugin.compiler.AbstractCompilerMojo.execute(AbstractCompilerMojo.java:1029) at org.apache.maven.plugin.compiler.TestCompilerMojo.execute(TestCompilerMojo.java:170) at org.apache.maven.plugin.DefaultBuildPluginManager.executeMojo(DefaultBuildPluginManager.java:134) at org.apache.maven.lifecycle.internal.MojoExecutor.execute(MojoExecutor.java:208) ... 31 more2017-03-01T00:13:08.632 [ERROR] 2017-03-01T00:13:08.632 [ERROR] Re-run Maven using the -X switch to enable full debug logging.2017-03-01T00:13:08.632 [ERROR] 2017-03-01T00:13:08.632 [ERROR] For more information about the errors and possible solutions, please read the following articles:2017-03-01T00:13:08.632 [ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoFailureException2017-03-01T00:13:08.632 [ERROR] 2017-03-01T00:13:08.632 [ERROR] After correcting the problems, you can resume the build with the command2017-03-01T00:13:08.632 [ERROR] mvn -rf :beam-runners-sparkchannel stoppedSetting status of 70c1864 to FAILURE with url https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/7987/ and message: 'Build finished. 'Using context: Jenkins: Maven clean install
--none--

asfbot · 2017-03-01T02:00:03Z

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

Failed Tests: 1

beam_PreCommit_Java_MavenInstall/org.apache.beam:beam-runners-spark: 1

org.apache.beam.runners.spark.translation.streaming.ResumeFromCheckpointStreamingTest.testWithResume

--none--

coveralls · 2017-03-01T02:00:42Z

Coverage increased (+0.07%) to 69.197% when pulling 0739a38 on jkff:style-guide into 352af85 on apache:master.

jkff · 2017-03-01T02:01:17Z

retest this please

coveralls · 2017-03-01T02:36:53Z

Coverage increased (+0.06%) to 69.193% when pulling 0739a38 on jkff:style-guide into 352af85 on apache:master.

asfbot · 2017-03-01T02:37:32Z

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

Failed Tests: 1

beam_PreCommit_Java_MavenInstall/org.apache.beam:beam-runners-apex: 1

org.apache.beam.runners.apex.examples.WordCountTest.testWordCountExample

--none--

jkff · 2017-03-01T02:38:17Z

OK, this failure is pretty clearly a flake, as well as the previous one.

dhalperi · 2017-03-01T03:40:50Z

+1 will merge when i get back. Any other committer welcome to preempt me

…

On Tue, Feb 28, 2017 at 18:38 Eugene Kirpichov ***@***.***> wrote: OK, this failure is pretty clearly a flake, as well as the previous one. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1943 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAgITyDEP2rNZgjJL0zLSNQLk9GjG8B_ks5rhNobgaJpZM4L6Q0m> .

dhalperi reviewed Feb 8, 2017

View reviewed changes

jkff force-pushed the style-guide branch from c37d550 to e4ff843 Compare February 8, 2017 02:03

jkff force-pushed the style-guide branch from e4ff843 to 6809d7f Compare February 9, 2017 00:37

dhalperi reviewed Feb 10, 2017

View reviewed changes

jkff force-pushed the style-guide branch from 3e46f94 to 74ab2e0 Compare February 28, 2017 03:16

jkff force-pushed the style-guide branch from 74ab2e0 to a3e26dd Compare February 28, 2017 22:13

dhalperi approved these changes Feb 28, 2017

View reviewed changes

jkff commented Feb 28, 2017

View reviewed changes

jkff added 8 commits February 28, 2017 16:34

BEAM-1420 GroupByKey should comply with PTransform style guide

3a32149

BEAM-1421 Latest should comply with PTransform style guide

5a9e9f2

BEAM-1423 Sample should comply with PTransform style guide

852317d

BEAM-1424 ToString should comply with PTransform style guide

c5bb4a4

BEAM-1426 SortValues should comply with PTransform style guide

1da4cd8

BEAM-1416 Write transform should comply with PTransform style guide

aca6300

BEAM-1419 Flatten should comply with PTransform style guide

e82d62c

BEAM-1417 Count should comply with PTransform style guide

0739a38

jkff force-pushed the style-guide branch from 70c1864 to 0739a38 Compare March 1, 2017 01:01

asfgit closed this in d66029c Mar 1, 2017

jkff deleted the style-guide branch March 1, 2017 19:24

Conversation

jkff commented Feb 8, 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!

asfbot commented Feb 8, 2017

Build result: FAILURE

Uh oh!

coveralls commented Feb 8, 2017

Uh oh!

asfbot commented Feb 8, 2017

Uh oh!

jkff commented Feb 9, 2017

Uh oh!

coveralls commented Feb 9, 2017

Uh oh!

coveralls commented Feb 9, 2017

Uh oh!

asfbot commented Feb 9, 2017

Uh oh!

asfbot commented Feb 9, 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!

coveralls commented Feb 28, 2017

Uh oh!

asfbot commented Feb 28, 2017

Uh oh!

coveralls commented Feb 28, 2017

Uh oh!

asfbot commented Feb 28, 2017

Uh oh!

dhalperi 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!

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!

jkff 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!

dhalperi commented Feb 28, 2017

Uh oh!

asfbot commented Mar 1, 2017

Build result: FAILURE

Uh oh!

asfbot commented Mar 1, 2017