Skip to content

Classify surviving deprecated storm-client APIs with forRemoval decisions (#8994) - #9172

Merged
reiabreu merged 1 commit into
apache:masterfrom
Mohit20057:classify-client-deprecated-apis
Oct 25, 2026
Merged

reiabreu merged 1 commit into
apache:masterfrom
Mohit20057:classify-client-deprecated-apis

Conversation

@Mohit20057

Copy link
Copy Markdown
Contributor

What is the purpose of the change

Sub-task of umbrella issue #8994.
Classifies surviving @Deprecated APIs, configurations, and utilities in storm-client by upgrading plain @Deprecated annotations to include explicit since = "..." versions and forRemoval decisions, and adds @deprecated Javadoc tags pointing to supported replacements. Configuration Config.TOPOLOGY_SPREAD_COMPONENTS, core interfaces/utilities (TimeCacheMap, IKryoDecorator, ILocalCluster, ILocalDRPC, IContext, INimbusCredentialPlugin), legacy Metrics V1 classes (CGroupMetricsBase and 5 subclasses, IMetricsContext, TopologyContext), and client constructors/factories (NimbusClient, SupervisorClient) are formally marked forRemoval = true. TransactionAttempt and Time.currentTimeSecs() / Time.deltaSecs() are marked forRemoval = false to preserve Kryo registration order and short relative duration use cases.

How was this tested

  • Built and compiled storm-client with JDK 23
  • Passed ConfigUtilsTest (25/25 tests passed)
  • Passed TestConfigValidate (38/38 tests passed)
  • Passed TimeTest (8/8 tests passed)
  • Passed SerializationFactoryTest (5/5 tests passed)
  • Checkstyle passed with 0 violations

@reiabreu reiabreu 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.

This review was generated with the help of an LLM (Claude) and reviewed by me before posting.

Claude spot-checked some of the new since values against git history: ILocalCluster (2.0.0) and CGroupMetricsBase (2.3.0) match, IKryoDecorator does not (inline). The other values weren't checked.

Comment thread storm-client/src/jvm/org/apache/storm/serialization/IKryoDecorator.java Outdated
@rzo1

rzo1 commented Oct 22, 2026

Copy link
Copy Markdown
Contributor

@Mohit20057 friendly ping: could you have a look at @reiabreu's review? The since value on IKryoDecorator should be 2.8.6 rather than 2.0.0. Thanks!

@Mohit20057

Copy link
Copy Markdown
Contributor Author

Hi @rzo1 @reiabreu,

Thanks for the review! I have updated the since attribute on IKryoDecorator#decorate(Kryo) to 2.8.6 in the latest commit to accurately match the Git history.

@rzo1
rzo1 force-pushed the classify-client-deprecated-apis branch from eba355b to 111c368 Compare October 22, 2026 19:38
@rzo1

rzo1 commented Oct 24, 2026

Copy link
Copy Markdown
Contributor

Thanks for working through these.

Checking against the release tags, several since values seem off:

  • NimbusClient and SupervisorClient constructors/factories: 2.6.4 (STORM-4070), not 2.0.0
  • TopologyContext.registerMetric/getRegisteredMetricByName: 2.0.0 (STORM-2909)
  • IMetricsContext.registerMetric: 2.1.0 (STORM-3371)
  • Config.TOPOLOGY_SPREAD_COMPONENTS: 2.2.0 (STORM-3585)
  • INimbusCredentialPlugin.populateCredentials(Map, Map): 2.0.0
  • TransactionAttempt: 2.0.0 (STORM-3156)
  • TimeCacheMap: after 0.8.0. I would leave since out, since there are no tags that old.

Two forRemoval=true decisions also look premature to me:

  • TOPOLOGY_SPREAD_COMPONENTS: the multitenant NodePool reads only this key, and TOPOLOGY_RAS_CONSTRAINTS applies only to the constraint solver.
  • TimeCacheMap is still used by CoordinatedBolt.

Either forRemoval=false for now, or migrate those usages first.

Minor: the CGroup V1 @deprecated tags could point to the matching org.apache.storm.metrics2.cgroup.* class rather than to the registry in general. The conflicts are from #9184 and should be mechanical.

@Mohit20057
Mohit20057 force-pushed the classify-client-deprecated-apis branch from 111c368 to 8297992 Compare October 25, 2026 16:27
@Mohit20057

Copy link
Copy Markdown
Contributor Author

@rzo1 Thanks for the thorough review !

I have updated the PR accordingly:

  1. Corrected all since values to match the release tags:
    • NimbusClient and SupervisorClient constructors/factories: 2.6.4
    • TopologyContext.registerMetric / getRegisteredMetricByName: 2.0.0
    • IMetricsContext.registerMetric: 2.1.0
    • Config.TOPOLOGY_SPREAD_COMPONENTS: 2.2.0
    • INimbusCredentialPlugin.populateCredentials(Map, Map): 2.0.0
    • TransactionAttempt: 2.0.0
    • TimeCacheMap: omitted since
  2. Changed forRemoval to false for TOPOLOGY_SPREAD_COMPONENTS and TimeCacheMap due to active internal usages.
  3. Updated the CGroup V1 @deprecated tags to link directly to their corresponding org.apache.storm.metrics2.cgroup.* classes.
  4. Rebased onto master, resolved the formatting conflicts from Checkstyle auto-fix of storm-client #9184, and verified mvn test-compile -pl storm-client passes cleanly with 0 Checkstyle violations under JDK 23.

@rzo1 rzo1 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 @Mohit20057, the since values match the release tags now and the forRemoval decisions look right to me. LGTM.

@reiabreu
reiabreu merged commit 057924e into apache:master Oct 25, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants