Checkstyle class-design cleanup: add private utility ctors, mark final, drop redundant modifiers, add missing @Override#1038
Merged
akshayrai merged 2 commits intoJun 29, 2026
Conversation
…l, drop redundant modifiers, add missing @OverRide Bundle of four mechanical class-design fixes the LinkedIn-internal checkstyle (a stricter superset of the OSS Checker) flags on the current OSS Brooklin tree. No behavior change in any file; every fix is a declaration-level no-op that documents intent the language already enforces, or that the class was implicitly relying on. None of the targeted rules are currently enabled in OSS Brooklin's own checkstyle.xml -- two of them (HideUtilityClassConstructorCheck, MissingOverrideCheck) are present-but-commented-out -- so this PR addresses the latent violations without proposing a config change. 49 fixes across 41 files, by rule: HideUtilityClassConstructorCheck (26) Adds an explicit `private ClassName()` constructor to each utility class (all-static-API classes) so the synthesized public default constructor doesn't accidentally let consumers instantiate them. Each ctor is a one-liner; no other changes in these files. Touched modules: datastream-{client, common, kafka, kafka-connector, server, testcommon, tools, utils}. FinalClassCheck (6) Marks five top-level classes (DatastreamRestClientCli, DynamicMetricsManager, MetricsTestUtils, EmbeddedDatastreamCluster, FutureUtils) and one nested class (KafkaMirrorMakerConnector.PartitionDiscoveryThread) `final`. Each already had only private constructors, so the `final` keyword just makes the existing not-meant-to-be-subclassed intent explicit -- the JVM was already enforcing it. RedundantModifierCheck (16) Drops the `public` modifier from constructors of nested types whose enclosing class is `private` (or, for ResettableGauge, whose own class is package-private). The `public` modifier on a constructor of a private/package-private type is redundant because the constructor cannot be invoked from anywhere the type itself isn't visible. Touched ZkAdapter's five private inner zk-listener classes, three CommonConnectorMetrics nested metric-bucket classes, Coordinator.CoordinatorMetrics, StickyPartitionAssignmentStrategy.ElasticTaskAssignmentInfo, RetryUtils.ExceptionTrackingMethodCaller, FileProcessor (package-private), BaseRestClientFactory.DaemonNamedThreadFactory, ResettableGauge x2, DatabaseChunkedReaderMetrics. MissingOverrideCheck (1) Adds @OverRide to AbstractKafkaConnector#getActiveTasks(), which already had `{@inheritdoc}` Javadoc but was missing the annotation the Javadoc implies. Motivation outside OSS Brooklin: brooklin-server consumes Brooklin in-tree (subtree-merge at brooklin-core/) and runs a stricter checkstyle on the merged code; the four rules above fire 49 times total today, forcing a build-level carve-out. With this PR merged upstream, brooklin-server's next OSS sync inherits the cleanup and the four rules drop out of its checkstyle-suppress list.
| @@ -10,6 +10,9 @@ | |||
| */ | |||
| public class DatastreamMetadataConstants { | |||
Collaborator
There was a problem hiding this comment.
Should we define this class also as final
| @@ -10,6 +10,9 @@ | |||
| */ | |||
| public class KafkaDatastreamMetadataConstants { | |||
Collaborator
There was a problem hiding this comment.
same, should this be defined a final class.
| @@ -23,6 +23,10 @@ | |||
| * Utility class for Kafka based connectors | |||
| */ | |||
| public class KafkaConnectorDiagUtils { | |||
Collaborator
There was a problem hiding this comment.
I think utils class should also be defined as final.
Collaborator
|
Thanks Akshay for sanitizing this. One nit: since we are picking this up, should we also consider defining all the Utils and Constant classes as final? |
…losure Follow-up to reviewer feedback on the previous commit: adding a private no-arg constructor to the 18 utility / constants classes flipped them from "synthesized public default ctor" to "only private ctors", which makes the existing FinalClassCheck rule fire on them (the rule requires final when a class has only private constructors and no descendants). The previous commit addressed FinalClass on 6 classes that were already in that state pre-patch; this commit closes the loop by marking the 18 classes whose new private ctor put them in the same state. No behavior change: every class was already non-extendable in practice (only private constructors). The final keyword just makes the intent explicit at the class declaration and satisfies the FinalClassCheck rule the previous commit otherwise leaves with new violations. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
harshOSS
approved these changes
Jun 29, 2026
akshayrai
added a commit
that referenced
this pull request
Jul 1, 2026
Two parallel PRs (#1038 'Checkstyle class-design cleanup' and #1040 'Centralize ObjectMapper construction via JsonUtils.newObjectMapper()') each independently added a private no-arg constructor to JsonUtils, merging into master with two definitions of the same constructor. This broke compilation of datastream-common with: error: constructor JsonUtils() is already defined in class JsonUtils Removed the duplicate, keeping the field declarations before the single remaining private constructor. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
No behavior changes in any file. All fixes are declaration-level no-ops that make existing intent explicit without changing runtime behavior.
Testing Done
No logic change. PR checks only