Skip to content
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

Merged
merged 4 commits into from
Oct 23, 2024
Merged

Conversation

sjhajharia
Copy link
Contributor

What

Code Cleanup in Streams Module

Changes

Some common changes include

  • Replace the Arrays.asList() with Collections.singletonList() wherever possible
  • Cleaning up some if-else blocks
  • Replacing some instances of String.builder() with String operations
  • Replcing some format() with printf()

Thanks!

@mjsax mjsax changed the title [MINOR] Code Cleanup - Streams MINOR: Code cleanup Kafka Streams Aug 23, 2024
Copy link
Member

@mjsax mjsax left a 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?

@mjsax
Copy link
Member

mjsax commented Sep 4, 2024

@sjhajharia -- do you still have interest to finish this PR?

@sjhajharia
Copy link
Contributor Author

Hey @mjsax
Sorry, I was swamped a bit. Getting back to this, thanks for the review. I have updated the PR.
Could you pls help review again.
Thanks

Copy link
Member

@mjsax mjsax left a 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")
Copy link
Member

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()));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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())
);

Copy link
Member

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);
Copy link
Member

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));
Copy link
Member

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)
            );

@mjsax mjsax merged commit 8faeb93 into apache:trunk Oct 23, 2024
6 of 7 checks passed
@mjsax
Copy link
Member

mjsax commented Oct 23, 2024

Thanks for the PR. Merged to trunk.

abhishekgiri23 pushed a commit to abhishekgiri23/kafka that referenced this pull request Nov 2, 2024
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