-
Notifications
You must be signed in to change notification settings - Fork 14k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
MINOR: Code cleanup Kafka Streams #16050
Conversation
ded2366
to
e705e3c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR. Couple of comments. Also, can you please rebase the PR to resolve merge conflict?
...ava/org/apache/kafka/streams/processor/internals/assignment/TaskAssignorConvergenceTest.java
Outdated
Show resolved
Hide resolved
...ava/org/apache/kafka/streams/processor/internals/assignment/TaskAssignorConvergenceTest.java
Outdated
Show resolved
Hide resolved
...c/test/java/org/apache/kafka/streams/processor/internals/assignment/AssignmentTestUtils.java
Outdated
Show resolved
Hide resolved
...c/test/java/org/apache/kafka/streams/processor/internals/assignment/AssignmentTestUtils.java
Outdated
Show resolved
Hide resolved
...c/test/java/org/apache/kafka/streams/processor/internals/assignment/AssignmentTestUtils.java
Outdated
Show resolved
Hide resolved
...c/test/java/org/apache/kafka/streams/processor/internals/assignment/AssignmentTestUtils.java
Outdated
Show resolved
Hide resolved
@sjhajharia -- do you still have interest to finish this PR? |
e705e3c
to
016cab2
Compare
Hey @mjsax |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR update. Made another pass. Just nits, but given it's a cleanup PR, guess it ok to be nitty, as the whole PR is?
Btw: I though using StringBuilder
is usually recommended as it more efficient? Does this not hold any longer? For this case, we are not on the critical code path so perf does not matter much, and I agree that StringBuilder
make the code harder to read... Just curious.
final List<KeyValue<String, String>> table2 = asList( | ||
new KeyValue<>("ID123", "BBB") | ||
final List<KeyValue<String, String>> table2 = Collections.singletonList( | ||
new KeyValue<>("ID123", "BBB") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: avoid unnecessary reformatting
.toString(), | ||
misassigned, | ||
is(emptyMap())); | ||
assertThat("Found some over- or under-assigned tasks in the final assignment with " + numStandbyReplicas + " and max warmups " + maxWarmupReplicas + " standby replicas, stateful tasks:" + statefulTasks + ", and stateless tasks:" + statelessTasks + failureContext, misassigned, is(emptyMap())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assertThat("Found some over- or under-assigned tasks in the final assignment with " + numStandbyReplicas + " and max warmups " + maxWarmupReplicas + " standby replicas, stateful tasks:" + statefulTasks + ", and stateless tasks:" + statelessTasks + failureContext, misassigned, is(emptyMap())); | |
assertThat( | |
"Found some over- or under-assigned tasks in the final assignment with " + numStandbyReplicas + | |
" and max warmups " + maxWarmupReplicas + " standby replicas, stateful tasks:" + statefulTasks + | |
", and stateless tasks:" + statelessTasks + failureContext, misassigned, | |
is(emptyMap()) | |
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line too long.. Let's do something like this to make it easier to read.
.append(failureContext) | ||
.toString() | ||
); | ||
throw new AssertionError("Found a standby task for stateless task " + standbyTask + " on client " + entry + " stateless tasks:" + statelessTasks + failureContext); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is boarderline... maybe better to split into two lines? (similar below)
StreamsConfig.consumerPrefix(ConsumerConfig.MAX_POLL_INTERVAL_MS_CONFIG), | ||
StreamsConfig.producerPrefix(ProducerConfig.RETRIES_CONFIG), | ||
StreamsConfig.producerPrefix(ProducerConfig.REQUEST_TIMEOUT_MS_CONFIG), | ||
StreamsConfig.producerPrefix(ProducerConfig.MAX_BLOCK_MS_CONFIG))); | ||
StreamsConfig.producerPrefix(ProducerConfig.MAX_BLOCK_MS_CONFIG)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Preferred formatting would be (but also ok to leave as-is):
System.err.printf(
"ERROR: Did not have all required configs expected to contain %s, %s, %s, %s%n",
StreamsConfig.consumerPrefix(ConsumerConfig.MAX_POLL_INTERVAL_MS_CONFIG),
StreamsConfig.producerPrefix(ProducerConfig.RETRIES_CONFIG),
StreamsConfig.producerPrefix(ProducerConfig.REQUEST_TIMEOUT_MS_CONFIG),
StreamsConfig.producerPrefix(ProducerConfig.MAX_BLOCK_MS_CONFIG)
);
016cab2
to
3fb0fc8
Compare
Thanks for the PR. Merged to |
Reviewers: Matthias J. Sax <[email protected]>
What
Code Cleanup in Streams Module
Changes
Some common changes include
Thanks!