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

KAFKA-18428: Measure share consumers performance #18415

Merged
merged 5 commits into from
Jan 8, 2025

Conversation

adixitconfluent
Copy link
Contributor

@adixitconfluent adixitconfluent commented Jan 7, 2025

About

Added code to measure performance of share consumer/consumers in a share group. Added the following files -

  1. ShareConsumerPerformance.java - Code which measures the performance of share consumer/consumers.
  2. ShareConsumerPerformanceTest.java - Contains unit tests for individual functionalities in ShareConsumerPerformance.java
  3. kafka-share-consumer-perf-test.sh - CLI utility to run ShareConsumerPerformance.java

@github-actions github-actions bot added triage PRs from the community tools labels Jan 7, 2025
@adixitconfluent adixitconfluent marked this pull request as draft January 7, 2025 13:31
@adixitconfluent adixitconfluent marked this pull request as ready for review January 7, 2025 13:43
@omkreddy omkreddy added ci-approved KIP-932 Queues for Kafka labels Jan 7, 2025
Copy link
Member

@AndrewJSchofield AndrewJSchofield 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. A few comments to resolve.

@@ -0,0 +1,20 @@
#!/bin/bash
Copy link
Member

Choose a reason for hiding this comment

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

Please can you also create bin/windows/kafka-share-consumer-perf-test.bat.

ShareConsumerPerfOptions options = new ShareConsumerPerfOptions(args);
AtomicLong totalMessagesRead = new AtomicLong(0);
AtomicLong totalBytesRead = new AtomicLong(0);
AtomicLong joinTimeMs = new AtomicLong(0);
Copy link
Member

Choose a reason for hiding this comment

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

joinTimeMs and joinTimeMsInSingleRound don't make sense for a share group consumer. In the regular consumer perf test, they are used to see how long it took to join the group. A share consumer cannot work this out because there is no onPartitionsAssigned callback. Please remove these variables throughout the code.

printStats(totalBytesRead.get(), totalMessagesRead.get(), elapsedSec, fetchTimeInMs, startMs, endMs,
options.dateFormat(), -1);

if (!shareConsumersMetrics.isEmpty()) {
Copy link
Member

Choose a reason for hiding this comment

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

I think this would be neater as shareConsumerMetrics.forEach.

totalBytesRead.set(bytesRead.get());
}

private static void consumeMessagesForSingleShareConsumer(long currentTimeMs,
Copy link
Member

Choose a reason for hiding this comment

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

This could be more neatly initialised by having lastConsumedTimeMs and currentTimeMs as local variables, and then initialising with

long currentTimeMs = System.currentTimeMills();
long lastConsumedTimeMs = currentTimeMs;

and then going into the while loop.

props.put(ConsumerConfig.BOOTSTRAP_SERVERS_CONFIG, brokerHostsAndPorts());
props.put(ConsumerConfig.GROUP_ID_CONFIG, options.valueOf(groupIdOpt));
props.put(ConsumerConfig.RECEIVE_BUFFER_CONFIG, options.valueOf(socketBufferSizeOpt).toString());
props.put(ConsumerConfig.MAX_PARTITION_FETCH_BYTES_CONFIG, options.valueOf(fetchSizeOpt).toString());
Copy link
Member

Choose a reason for hiding this comment

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

This would be FETCH_MAX_BYTES_CONFIG for a share consumer I think.

Copy link
Member

@AndrewJSchofield AndrewJSchofield left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@AndrewJSchofield AndrewJSchofield merged commit b51b31e into apache:trunk Jan 8, 2025
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci-approved KIP-932 Queues for Kafka tools triage PRs from the community
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants