KAFKA-19624: Improving consistency of command-line arguments for consumer performance tests #20385

aheev · 2025-08-20T14:40:54Z

resolves https://issues.apache.org/jira/browse/KAFKA-19624

Reviewers: @brandboat, @AndrewJSchofield, @m1a2st

…umer performance tests

aheev · 2025-08-20T14:41:28Z

@AndrewJSchofield can you please review this?

brandboat · 2025-08-25T02:11:38Z

ping @aheev, could you please fix the conflicts?

# Conflicts: # tools/src/main/java/org/apache/kafka/tools/ConsumerPerformance.java # tools/src/test/java/org/apache/kafka/tools/ConsumerPerformanceTest.java

aheev · 2025-08-25T03:07:42Z

ping @aheev, could you please fix the conflicts?

done

brandboat

Thanks for the patch, left some comments below.

tools/src/main/java/org/apache/kafka/tools/ConsumerPerformance.java

tools/src/main/java/org/apache/kafka/tools/ShareConsumerPerformance.java

aheev · 2025-08-25T07:16:09Z

FAILED ❌ RestoreIntegrationTest > "shouldInvokeUserDefinedGlobalStateRestoreListener(boolean).useNewProtocol=false"

This test seems to be flaky. It succeeds on my machine and fails sometimes. Also the changes are not even related to the test

Just tested. Same issue on trunk too

tools/src/main/java/org/apache/kafka/tools/ConsumerPerformance.java

m1a2st

Thanks for this patch, some comments left

tools/src/main/java/org/apache/kafka/tools/ConsumerPerformance.java

AndrewJSchofield

Thanks for the PR. Please add some tests for the variation combinations of new/old options to make sure the rules in the KIP are correctly implemented.

tests/kafkatest/services/performance/consumer_performance.py

tests/kafkatest/services/performance/share_consumer_performance.py

tools/src/main/java/org/apache/kafka/tools/ConsumerPerformance.java

tools/src/main/java/org/apache/kafka/tools/ShareConsumerPerformance.java

…nd `messages` traces from the tools

brandboat

Thanks for the update, some minor comments.

tools/src/main/java/org/apache/kafka/tools/ShareConsumerPerformance.java

brandboat · 2025-08-27T09:03:50Z

tools/src/test/java/org/apache/kafka/tools/ConsumerPerformanceTest.java

@@ -136,7 +174,9 @@ public void testConfigWithoutTopicAndInclude() {

    @Test
    public void testClientIdOverride() throws IOException {
-        File tempFile = Files.createFile(tempDir.resolve("test_consumer_config.conf")).toFile();
+        Path configPath = tempDir.resolve("test_consumer_config.conf");
+        Files.deleteIfExists(configPath);


This is not required, tempDir will be cleanup automatically after junit test finished.
Same thing in other places.

While I was testing, a test ended abruptly and caused FileAlreadyExists exceptions on next runs. Hence, I have added this code

I see, we have multiple test files with the same name. We could give them distinct names—for example, by adding a random suffix or simply choosing different names. This is just my personal nit though.

Changed the file names to test_name_test_class.conf

tools/src/test/java/org/apache/kafka/tools/ConsumerPerformanceTest.java

tools/src/test/java/org/apache/kafka/tools/ShareConsumerPerformanceTest.java

brandboat

LGTM, thanks!

tools/src/test/java/org/apache/kafka/tools/ConsumerPerformanceTest.java

AndrewJSchofield

Thanks for the updates.

I see a NullPointerException from kafka-consumer-perf-test.sh if I fail to provide --bootstrap-server. Since that's a required argument, I would expect an error message.

tools/src/main/java/org/apache/kafka/tools/ConsumerPerformance.java

AndrewJSchofield

Apart from adding --command-property to both kafka-consumer-perf-test.sh and kafka-share-consumer-perf-test.sh, this looks good to me.

AndrewJSchofield · 2025-09-01T16:26:29Z

tools/src/main/java/org/apache/kafka/tools/ConsumerPerformance.java

+            commandPropertiesOpt = parser.accepts("command-property", "Kafka consumer related configuration properties like client.id. " +
+                    "These configs take precedence over those passed via --command-config or --consumer.config.")
+                .withRequiredArg()
+                .describedAs("prop1=val1,prop2=val2...")


I do not think the code handles the format as described. For example, if I use --command-property client.id=c1,check.crcs=false, then the client ID is "c1,check.crcs=false". The tricky part is that kafka-producer-perf-test uses a different argument parsing library and it supports --command-property client.id=c1 bootstrap.servers=localhost:9092 with a space used as a separator. That's very non-standard but it's already there for that tool at least.

Personally, I think that the easiest is to support prop=val only and permit multiple instances of --command-property, so someone can do --command-property client.id=c1 --command-property check.crcs=false. I didn't think the details would be subtle. wdyt?

The same comment applies to the share consumer perf test also.

makes sense as the option name implies a single prop. I will add the change, test it and ask for review

aheev · 2025-09-02T13:01:08Z

Overall, LGTM. Could you run the e2e test and share the results?

What do you mean by e2e test?

m1a2st · 2025-09-02T13:14:19Z

Since you have modified the e2e test file consumer_performance.py, you can validate it by running the following command to ensure the test still works:
TC_PATHS="tests/kafkatest/benchmarks/core/benchmark_test.py" bash tests/docker/run_tests.sh

FYI: https://github.com/apache/kafka/blob/trunk/tests/README.md

aheev · 2025-09-02T13:21:57Z

Since you have modified the e2e test file consumer_performance.py, you can validate it by running the following command to ensure the test still works: TC_PATHS="tests/kafkatest/benchmarks/core/benchmark_test.py" bash tests/docker/run_tests.sh

FYI: https://github.com/apache/kafka/blob/trunk/tests/README.md

I will run them after andrew's comments are resolved

AndrewJSchofield · 2025-09-03T10:05:39Z

@aheev Please resolve conflicts

AndrewJSchofield

Thanks for the PR. Once we have a green build, I'm ready to merge this.

aheev · 2025-09-03T12:12:25Z

Thanks for the PR. Once we have a green build, I'm ready to merge this.

Shouldn't we wait for this? I am trying to run the tests, but running into some issues

Since you have modified the e2e test file consumer_performance.py, you can validate it by running the following command to ensure the test still works:
TC_PATHS="tests/kafkatest/benchmarks/core/benchmark_test.py" bash tests/docker/run_tests.sh

# Conflicts: # tools/src/main/java/org/apache/kafka/tools/ConsumerPerformance.java

AndrewJSchofield · 2025-09-03T14:41:01Z

Thanks for the PR. Once we have a green build, I'm ready to merge this.

Shouldn't we wait for this? I am trying to run the tests, but running into some issues

Since you have modified the e2e test file consumer_performance.py, you can validate it by running the following command to ensure the test still works:
TC_PATHS="tests/kafkatest/benchmarks/core/benchmark_test.py" bash tests/docker/run_tests.sh

Yes, we should. I'll wait then.

aheev · 2025-09-04T16:57:31Z

After a lot of struggle I managed to run the test by using a different JDK(will file a ticket for this later). Here are the results @m1a2st . Three tests failed, all of which are related to producer-perf
benchmark_test.log

m1a2st · 2025-09-05T01:00:57Z

Here are the results @m1a2st . Three tests failed, all of which are related to producer-perf

Thanks, Could you file a ticket to trace this issue?

# Conflicts: # tools/src/main/java/org/apache/kafka/tools/ShareConsumerPerformance.java

aheev · 2025-09-05T06:53:22Z

Here are the results @m1a2st . Three tests failed, all of which are related to producer-perf

Thanks, Could you file a ticket to trace this issue?

https://issues.apache.org/jira/browse/KAFKA-19672

aheev · 2025-09-05T08:37:21Z

This test seems to be flaky and not related to the changes. It succeeded in a couple of runs in my local. @AndrewJSchofield should we trigger a CI re-run?

FAILED ❌ SmokeTestDriverIntegrationTest > "shouldWorkWithRebalance(boolean, boolean, boolean).stateUpdaterEnabled=false, processingThreadsEnabled=false, streamsProtocolEnabled=true"
FAILED ❌ SmokeTestDriverIntegrationTest > "shouldWorkWithRebalance(boolean, boolean, boolean).stateUpdaterEnabled=true, processingThreadsEnabled=false, streamsProtocolEnabled=true"
FAILED ❌ SmokeTestDriverIntegrationTest > "shouldWorkWithRebalance(boolean, boolean, boolean).stateUpdaterEnabled=true, processingThreadsEnabled=true, streamsProtocolEnabled=true"
Found 2 flaky test failures:
FLAKY ⚠️  SmokeTestDriverIntegrationTest > "shouldWorkWithRebalance(boolean, boolean, boolean).stateUpdaterEnabled=true, processingThreadsEnabled=true, streamsProtocolEnabled=false"
FLAKY ⚠️  SmokeTestDriverIntegrationTest > "shouldWorkWithRebalance(boolean, boolean, boolean).stateUpdaterEnabled=true, processingThreadsEnabled=false, streamsProtocolEnabled=false"

AndrewJSchofield · 2025-09-05T15:48:50Z

@aheev I would merge latest changes into this branch. I find the test fails on your branch, and passes on trunk. I can't see that this PR would cause the tests to fail, so I'd update the branch and let the CI run again.

tools/src/test/java/org/apache/kafka/tools/ConsumerPerformanceTest.java

Yunyung · 2025-09-05T16:10:38Z

tests/kafkatest/services/performance/consumer_performance.py

@@ -115,7 +115,7 @@ def start_cmd(self, node):
        for key, value in self.args(node.version).items():
            cmd += " --%s %s" % (key, value)

-        cmd += " --consumer.config %s" % ConsumerPerformanceService.CONFIG_FILE
+        cmd += " --command-config %s" % ConsumerPerformanceService.CONFIG_FILE


Will this break e2e testing with older versions? (See #20471)

Yunyung · 2025-09-05T16:11:32Z

tests/kafkatest/services/performance/share_consumer_performance.py

@@ -100,7 +100,7 @@ def start_cmd(self, node):
        for key, value in self.args().items():
            cmd += " --%s %s" % (key, value)

-        cmd += " --consumer.config %s" % ShareConsumerPerformanceService.CONFIG_FILE
+        cmd += " --command-config %s" % ShareConsumerPerformanceService.CONFIG_FILE


Ditto. The shared consumer is very new, so maybe it’s not a big deal. I’m not sure though.

Share consumer was in 4.1 and we are now in 4.2.

…losed

KAFKA-19487: Improving consistency of command-line arguments for cons…

931dfaf

…umer performance tests

github-actions bot added triage PRs from the community tools labels Aug 20, 2025

aheev changed the title ~~KAFKA-19487: Improving consistency of command-line arguments for consumer performance tests~~ KAFKA-19624: Improving consistency of command-line arguments for consumer performance tests Aug 20, 2025

AndrewJSchofield added ci-approved and removed triage PRs from the community labels Aug 22, 2025

Merge branch 'trunk' of https://github.com/apache/kafka into KAFKA-19487

17460c7

# Conflicts: # tools/src/main/java/org/apache/kafka/tools/ConsumerPerformance.java # tools/src/test/java/org/apache/kafka/tools/ConsumerPerformanceTest.java

brandboat reviewed Aug 25, 2025

View reviewed changes

tools/src/main/java/org/apache/kafka/tools/ConsumerPerformance.java Show resolved Hide resolved

tools/src/main/java/org/apache/kafka/tools/ShareConsumerPerformance.java Show resolved Hide resolved

Add deprecation cycle for num-records

34651a9

aheev requested a review from brandboat August 25, 2025 06:27

brandboat reviewed Aug 25, 2025

View reviewed changes

tools/src/main/java/org/apache/kafka/tools/ConsumerPerformance.java Outdated Show resolved Hide resolved

tools/src/main/java/org/apache/kafka/tools/ConsumerPerformance.java Outdated Show resolved Hide resolved

m1a2st reviewed Aug 25, 2025

View reviewed changes

tools/src/main/java/org/apache/kafka/tools/ConsumerPerformance.java Show resolved Hide resolved

tools/src/main/java/org/apache/kafka/tools/ConsumerPerformance.java Outdated Show resolved Hide resolved

AndrewJSchofield self-requested a review August 25, 2025 14:42

AndrewJSchofield requested changes Aug 25, 2025

View reviewed changes

aheev added 4 commits August 26, 2025 16:32

Merge branch 'trunk' of https://github.com/apache/kafka into KAFKA-19487

529eb36

Add deprecation signatures to old options; Remove consumer config a…

d82ce4f

…nd `messages` traces from the tools

Add checks for deprecated options

c4cd51e

Merge branch 'trunk' of https://github.com/apache/kafka into KAFKA-19487

b8b2dc4

aheev requested review from brandboat, m1a2st and AndrewJSchofield August 26, 2025 20:01

brandboat reviewed Aug 27, 2025

View reviewed changes

aheev added 2 commits August 27, 2025 14:55

Rename tests to old config names

5c8fd2b

Merge branch 'trunk' of https://github.com/apache/kafka into KAFKA-19487

d252457

brandboat approved these changes Aug 27, 2025

View reviewed changes

tools/src/test/java/org/apache/kafka/tools/ConsumerPerformanceTest.java Outdated Show resolved Hide resolved

Rename consumer config files to unique prefix-suffixes

9d9a8c6

replace consumer config -> config in performance service files

9e53ae5

AndrewJSchofield requested changes Aug 28, 2025

View reviewed changes

AndrewJSchofield reviewed Aug 28, 2025

View reviewed changes

tools/src/main/java/org/apache/kafka/tools/ConsumerPerformance.java Show resolved Hide resolved

aheev added 2 commits August 28, 2025 21:16

add required check on bootstrap server opt

7020430

add required check on bootstrap server opt tests

a99f389

AndrewJSchofield requested changes Aug 28, 2025

View reviewed changes

aheev added 2 commits September 1, 2025 16:47

Add command-property

a6b1d7d

Merge branch 'trunk' of https://github.com/apache/kafka into KAFKA-19487

0f06d95

AndrewJSchofield requested changes Sep 1, 2025

View reviewed changes

aheev added 2 commits September 2, 2025 21:52

fix command-property description

4ff0992

Merge branch 'trunk' of https://github.com/apache/kafka into KAFKA-19487

a36a868

aheev requested a review from AndrewJSchofield September 2, 2025 16:48

AndrewJSchofield approved these changes Sep 3, 2025

View reviewed changes

Merge branch 'trunk' of https://github.com/apache/kafka into KAFKA-19487

5af4ee4

# Conflicts: # tools/src/main/java/org/apache/kafka/tools/ConsumerPerformance.java

Merge branch 'trunk' of https://github.com/apache/kafka into KAFKA-19487

fc98942

# Conflicts: # tools/src/main/java/org/apache/kafka/tools/ShareConsumerPerformance.java

Yunyung reviewed Sep 5, 2025

View reviewed changes

tools/src/test/java/org/apache/kafka/tools/ConsumerPerformanceTest.java Show resolved Hide resolved

Yunyung reviewed Sep 5, 2025

View reviewed changes

aheev added 2 commits September 6, 2025 01:03

Merge branch 'trunk' of https://github.com/apache/kafka into KAFKA-19487

116f7cd

rename messages to num-records in testMetricsRetrievedBeforeConsumerC…

d9f8854

…losed

KAFKA-19624: Improving consistency of command-line arguments for consumer performance tests #20385

Are you sure you want to change the base?

KAFKA-19624: Improving consistency of command-line arguments for consumer performance tests #20385

Conversation

aheev commented Aug 20, 2025 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aheev commented Aug 20, 2025

Uh oh!

brandboat commented Aug 25, 2025

Uh oh!

aheev commented Aug 25, 2025

Uh oh!

brandboat left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

aheev commented Aug 25, 2025

Uh oh!

Uh oh!

Uh oh!

m1a2st left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

AndrewJSchofield left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

brandboat left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

brandboat Aug 27, 2025

Choose a reason for hiding this comment

Uh oh!

aheev Aug 27, 2025

Choose a reason for hiding this comment

Uh oh!

brandboat Aug 27, 2025

Choose a reason for hiding this comment

Uh oh!

aheev Aug 27, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

brandboat left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AndrewJSchofield left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AndrewJSchofield left a comment

Choose a reason for hiding this comment

Uh oh!

AndrewJSchofield Sep 1, 2025

Choose a reason for hiding this comment

Uh oh!

aheev Sep 2, 2025

Choose a reason for hiding this comment

Uh oh!

aheev commented Aug 20, 2025 •

edited by github-actions bot

Loading

aheev commented Sep 2, 2025 •

edited

Loading

Yunyung Sep 5, 2025 •

edited

Loading

Yunyung Sep 5, 2025 •

edited

Loading