Skip to content

MINOR: Code cleanup Kafka Streams#16050

Merged
mjsax merged 4 commits intoapache:trunkfrom
sjhajharia:streams-code-cleanup
Oct 23, 2024
Merged

MINOR: Code cleanup Kafka Streams#16050
mjsax merged 4 commits intoapache:trunkfrom
sjhajharia:streams-code-cleanup

Conversation

@sjhajharia
Copy link
Copy Markdown
Collaborator

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!

@sjhajharia sjhajharia force-pushed the streams-code-cleanup branch from ded2366 to e705e3c Compare June 18, 2024 06:53
@mjsax mjsax changed the title [MINOR] Code Cleanup - Streams MINOR: Code cleanup Kafka Streams Aug 23, 2024
Copy link
Copy Markdown
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
Copy Markdown
Member

mjsax commented Sep 4, 2024

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

@sjhajharia
Copy link
Copy Markdown
Collaborator 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

@sjhajharia sjhajharia requested a review from mjsax September 21, 2024 12:19
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.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
Copy Markdown
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)
            );

@sjhajharia sjhajharia force-pushed the streams-code-cleanup branch from 016cab2 to 3fb0fc8 Compare October 7, 2024 06:55
@mjsax mjsax merged commit 8faeb93 into apache:trunk Oct 23, 2024
@mjsax
Copy link
Copy Markdown
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
Reviewers: Matthias J. Sax <matthias@confluent.io>
chiacyu pushed a commit to chiacyu/kafka that referenced this pull request Nov 30, 2024
Reviewers: Matthias J. Sax <matthias@confluent.io>
tedyu pushed a commit to tedyu/kafka that referenced this pull request Jan 6, 2025
Reviewers: Matthias J. Sax <matthias@confluent.io>
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