[BEAM-2306] Fail build when @Deprecated is used without @deprecated javadoc by alex-filatov · Pull Request #3531 · apache/beam

alex-filatov · 2017-07-10T10:27:34Z

Add checkstyle check to fail the build when @Deprecated is used without @deprecated javadoc (or vice versa).

The check is disabled for existing violations where reason for deprecation and/or alternative is not clear.

@deprecated

…s used without @deprecated javadoc (or vice versa). The check is disabled for existing violations where reason for deprecation and/or alternative is not clear.

coveralls · 2017-07-10T11:42:05Z

Coverage decreased (-0.009%) to 70.963% when pulling 0694dcc on alex-filatov:beam-2306-check-missing-deprecated into 9f904dc on apache:master.

alex-filatov · 2017-07-10T11:45:51Z

R: @kennknowles

kennknowles

Thanks! I've added a lot of content, so we can just not have any undocumented deprecation left.

kennknowles · 2017-07-10T17:22:18Z

runners/core-java/src/main/java/org/apache/beam/runners/core/InMemoryTimerInternals.java

  }

+  /**
+   * @deprecated use {@link #setTimer(StateNamespace, String, Instant, TimeDomain)}.


I think here you can do @inheritdoc so you don't have to spell it out again. And same throughout.

Unfortunately doesn't work. Only main description, @param, @returns, @throws are inheritable.

Ah, too bad. Thanks for clarifying.

kennknowles · 2017-07-10T17:24:11Z

...rc/main/java/org/apache/beam/runners/core/construction/CreatePCollectionViewTranslation.java

            PCollectionView.class.getSimpleName());
  }

+  // CHECKSTYLE.OFF: MissingDeprecated


Thanks for putting this together! I will just provide commentary for these so you don't have to use exclusions.

@deprecated runners should move away from translating `CreatePCollectionView` and treat this as part of the translation for a `ParDo` side input.

kennknowles · 2017-07-10T17:24:25Z

...rc/main/java/org/apache/beam/runners/core/construction/CreatePCollectionViewTranslation.java

  }

  /** Registers {@link CreatePCollectionViewTranslator}. */
+  // CHECKSTYLE.OFF: MissingDeprecated


Same as above.

kennknowles · 2017-07-10T17:25:28Z

...oogle-cloud-dataflow-java/src/main/java/org/apache/beam/runners/dataflow/DataflowRunner.java

   * A marker {@link DoFn} for writing the contents of a {@link PCollection} to a streaming
   * {@link PCollectionView} backend implementation.
   */
+  // CHECKSTYLE.OFF: MissingDeprecated


I think this @Deprecated can just be switched to @Internal. The important thing is that this is not for users anyhow.

kennknowles · 2017-07-10T17:29:32Z

sdks/java/core/src/main/java/org/apache/beam/sdk/coders/Coder.java

 */
 public abstract class Coder<T> implements Serializable {
  /** The context in which encoding or decoding is being done. */
+  // CHECKSTYLE.OFF: MissingDeprecated


@deprecated to implement a coder, do not use any `Context`. Just implement only those abstract methods which do not accept a `Context` and leave the default implementations for methods accepting a `Context`.

kennknowles · 2017-07-10T17:39:22Z

sdks/java/core/src/main/java/org/apache/beam/sdk/coders/CoderRegistry.java

   */
+  // CHECKSTYLE.OFF: MissingDeprecated
  @Deprecated
+  // CHECKSTYLE.ON: MissingDeprecated


@deprecated `getCoderFromDescriptor(TypeDescriptor, Map)

There is no intended alternative, this is a visibility issue and this code should be moved into the CombineFns class.

So purpose of the deprecation here is to attract attention to the @Internal annotation? In this case I propose to remove deprecation for consistency with the remaining codebase.

See above comment - just deprecated to indicate that it is going to change. So even though it is @Internal (and the javadoc should be loud about this, like in other places) it is helpful for contributors to see their IDEs strikethrough the method.

kennknowles · 2017-07-10T17:39:26Z

sdks/java/core/src/main/java/org/apache/beam/sdk/coders/CoderRegistry.java

   */
+  // CHECKSTYLE.OFF: MissingDeprecated
  @Deprecated
+  // CHECKSTYLE.ON: MissingDeprecated


@deprecated `getCoderFromDescriptor(TypeDescriptor, Map)

getCoderFromDescriptor doesn't exist, do you mean the private getCoderFromTypeDescriptor?.

Also, this method has no replacement and is really meant to be copied into ParDo having ParDo own all that logic.

Yea, I mean getCoderFromTypeDescriptor. Basically we do need at least one (and probably exactly one) public method where you can pass in a context of type-to-coder bindings. And the rest should be directed to use that.

Ken, have you seen enough demand for such a function in user based transforms that warrants visibility vs having a few independent copies?

I don't think it can be implemented outside of the registry.

Maybe if all the uses are just special cases that don't require real coder inference, that could be fine. But this method itself is quite simple so probably specialized versions would add complexity. I'm open to whatever, though.

If you don't believe that is possible, we should change the @deprecated comment to be something like:
@deprecated This method is to change in an unknown backwards incompatible way once support for this functionality is refined.

Similarly for the other CoderRegistry @Internal/@Deprecated methods.

Sounds good to me

kennknowles · 2017-07-10T18:38:41Z

sdks/java/core/src/main/java/org/apache/beam/sdk/testing/StreamingIT.java

 *     }
 * </code></pre>
 */
+// CHECKSTYLE.OFF: MissingDeprecated


@deprecated tests which use unbounded PCollections should be in the category {@link UsesUnboundedPCollections}. Beyond that, it is up to the runner and test configuration to decide whether to run in streaming mode.

kennknowles · 2017-07-10T18:41:14Z

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


+  // CHECKSTYLE.OFF: MissingDeprecated
  @Deprecated
+  // CHECKSTYLE.ON: MissingDeprecated


I actually don't think this should be deprecated.

kennknowles · 2017-07-10T18:56:11Z

sdks/java/core/src/main/java/org/apache/beam/sdk/values/PCollectionViews.java

    /**
     * Returns if a default value was specified.
     */
+    // CHECKSTYLE.OFF: MissingDeprecated


@deprecated these methods are all expected to change arbitrarily

But really, @Internal is probably enough.

coveralls · 2017-07-11T09:39:24Z

Coverage decreased (-0.1%) to 70.845% when pulling f1a3e19 on alex-filatov:beam-2306-check-missing-deprecated into 9f904dc on apache:master.

kennknowles · 2017-07-11T16:39:52Z

runners/core-java/src/main/java/org/apache/beam/runners/core/InMemoryTimerInternals.java

  }

+  /**
+   * @deprecated use {@link #setTimer(StateNamespace, String, Instant, TimeDomain)}.


Ah, too bad. Thanks for clarifying.

kennknowles · 2017-07-11T16:40:33Z

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


+  // CHECKSTYLE.OFF: MissingDeprecated
  @Deprecated
+  // CHECKSTYLE.ON: MissingDeprecated


This is another case of the CreatePCollectionView transform being deprecated entirely.

kennknowles · 2017-07-11T16:41:07Z

sdks/java/core/src/main/java/org/apache/beam/sdk/coders/CoderRegistry.java

   */
+  // CHECKSTYLE.OFF: MissingDeprecated
  @Deprecated
+  // CHECKSTYLE.ON: MissingDeprecated


Sounds good to me

kennknowles · 2017-07-11T16:41:49Z

sdks/java/core/src/main/java/org/apache/beam/sdk/coders/CoderRegistry.java

   */
+  // CHECKSTYLE.OFF: MissingDeprecated
  @Deprecated
+  // CHECKSTYLE.ON: MissingDeprecated


See above comment - just deprecated to indicate that it is going to change. So even though it is @Internal (and the javadoc should be loud about this, like in other places) it is helpful for contributors to see their IDEs strikethrough the method.

coveralls · 2017-07-11T22:07:56Z

Coverage decreased (-0.1%) to 70.841% when pulling 499bd87 on alex-filatov:beam-2306-check-missing-deprecated into 9f904dc on apache:master.

alex-filatov · 2017-07-12T08:27:20Z

All done. Please take a look.

coveralls · 2017-07-19T14:36:14Z

Coverage increased (+0.008%) to 70.624% when pulling dfe9326 on alex-filatov:beam-2306-check-missing-deprecated into 7fde976 on apache:master.

kennknowles

LGTM. Nice!

jkff · 2017-07-22T00:57:30Z

This PR broke Dataflow runner because it deleted this code:
https://github.com/apache/beam/pull/3531/files#diff-59c03929b0864dc394262c8c6d19880bL240
The comments on that code should have been more clear, like "Do not delete this without running Dataflow ValidatesRunner tests" :)

alex-filatov · 2017-07-22T07:07:47Z

Sorry for this. I couldn't find usage of this code in the repo, so assumed it was safe to delete.

kennknowles · 2017-07-24T00:58:00Z

No need to apologize. Your PR was just fine. It is on us to either make it a core part of the API or document why it exists despite being unused. Since it was leftover for compatibility, one way to make it obvious would be with a test in DataflowRunner.

And [partial] rollbacks are painless - don't view it as indicating any fault. A large number of our tests do not run until postcommit (to avoid massive amounts of computation) so it is expected for such things to happen fairly often.

[BEAM-2306] Add checkstyle check to fail the build when @deprecated i…

0694dcc

…s used without @deprecated javadoc (or vice versa). The check is disabled for existing violations where reason for deprecation and/or alternative is not clear.

kennknowles requested changes Jul 10, 2017

View reviewed changes

[BEAM-2306] Add missing @deprecated javadocs.

f1a3e19

kennknowles reviewed Jul 11, 2017

View reviewed changes

[BEAM-2306] Add missing @deprecated javadocs.

499bd87

Merge branch 'master' into beam-2306-check-missing-deprecated

dfe9326

kennknowles approved these changes Jul 19, 2017

View reviewed changes

asfgit closed this in a6f460f Jul 19, 2017

alex-filatov deleted the beam-2306-check-missing-deprecated branch October 19, 2017 15:28

Conversation

alex-filatov commented Jul 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Jul 10, 2017

Uh oh!

alex-filatov commented Jul 10, 2017

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!

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 Jul 11, 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 Jul 11, 2017

Uh oh!

alex-filatov commented Jul 12, 2017

Uh oh!

coveralls commented Jul 19, 2017

Uh oh!

kennknowles left a comment

Choose a reason for hiding this comment

Uh oh!

jkff commented Jul 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alex-filatov commented Jul 22, 2017

Uh oh!

kennknowles commented Jul 24, 2017

alex-filatov commented Jul 10, 2017 •

edited

Loading

jkff commented Jul 22, 2017 •

edited

Loading