Checkstyle code-safety cleanup: avoid parameter reassignment and null-side equals#1039
Merged
akshayrai merged 1 commit intoJun 29, 2026
Merged
Conversation
…-side equals
Bundle of two mechanical code-safety 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. Neither
rule is currently enabled in OSS Brooklin's own checkstyle.xml, so
this PR addresses the latent violations without proposing a config
change.
21 fixes across 12 files, by rule:
ParameterAssignmentCheck (18)
Reassigning a method parameter inside its body is a long-standing
readability/debuggability footgun -- the parameter name silently
changes meaning partway through the method, debugger watches and
stack traces lie, and overloading semantics get confusing for
derived classes. The mechanical fix is to introduce a final-by-
convention local that captures the transformed value and then
use the local for the rest of the method. Touched sites:
BaseRestClientFactory.{registerRestClient,addOverride,getOverride}
-- 'uri' was being clobbered by sanitizeUri; introduce
sanitizedUri local.
FieldMetadata.parseMetadata -- 'meta' was being trimmed of its
trailing delimiter; introduce trimmedMeta local and fold the
if/assign into a ternary.
CommonConnectorMetrics.{getEventProcessingMetrics,
getEventPollMetrics, getPartitionSpecificMetrics} -- the
`prefix = Strings.nullToEmpty(prefix); return X(prefix)`
pattern was 3 lines, fold into a single expression.
KafkaDestination.parse -- 'uri' was being clobbered by URIUtil.decode
inside a try block whose catch still wants to log the original
URI; introduce decodedUri local. (No behavior change: the catch
already used the pre-decode uri because the assignment is the
throwing statement.)
AbstractKafkaBasedConnectorTask.containsTransientException --
'ex' was being walked up the cause chain; rename loop variable
to 'cause' so the method's input parameter stays meaningful.
KafkaBasedConnectorTaskMetrics.getKafkaBasedConnectorTaskSpecificMetrics
-- 'prefix' was being clobbered by Strings.nullToEmpty;
introduce safePrefix local used in all 11 metric registrations.
DatastreamProducerRecordBuilder.addEvent -- 'key'/'value' were
being null-coalesced; introduce eventKey/eventValue locals.
ZkClient.ensurePath -- 'path' was being walked up the ZK tree
one separator at a time; introduce 'current' local so the
parameter is preserved.
RestliUtils.sanitizeUri -- 'uri' was being lowercased and
potentially scheme-prefixed; introduce 'sanitized' local.
RestliUtils.withPaging -- 'stream' was being replaced with
.skip()/.limit() variants; introduce 'paged' local.
EqualsAvoidNullCheck (3)
Calls of the form `variable.equals("literal")` throw NPE when
the variable is null, where `"literal".equals(variable)` simply
returns false. The fix is to flip the comparison so the
string literal is the receiver. Touched sites:
DatastreamTaskImpl.getDatastreamTaskName -- `_id.equals("")`
-> `"".equals(_id)`.
BrokenConnector / DummyConnector ctors -- the dummy-config
validation `dummyConfigValue.equals("dummyValue")` ->
`"dummyValue".equals(dummyConfigValue)`. (In both cases
the variable is currently non-null because it comes from
Properties.getProperty(name, ""), but the rule still fires
and flipping is harmless.)
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 two rules above fire 21 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 two rules drop out of its checkstyle-suppress list.
| @JsonIgnore | ||
| public String getDatastreamTaskName() { | ||
| return _id.equals("") ? _taskPrefix : _taskPrefix + "_" + _id; | ||
| return "".equals(_id) ? _taskPrefix : _taskPrefix + "_" + _id; |
Collaborator
There was a problem hiding this comment.
nit: Should we use StringUtils.isEmpty() for better readability
Collaborator
Author
There was a problem hiding this comment.
@dhananjay-sawner I'd rather do that in a separate PR. Since this is a wide scanning PR, I want to keep it simple and just address checkstyle issue here.
Collaborator
There was a problem hiding this comment.
Sure, looks good
harshOSS
approved these changes
Jun 29, 2026
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
This PR makes mechanical, no-op refactors to resolve latent Checkstyle violations without changing behavior or enabling any new rules in OSS Brooklin's checkstyle.xml.
Testing Done
No change in logic, PR checks only