-
Notifications
You must be signed in to change notification settings - Fork 14.6k
KAFKA-19624: Improving consistency of command-line arguments for consumer performance tests #20385
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
base: trunk
Are you sure you want to change the base?
Conversation
…umer performance tests
@AndrewJSchofield can you please review this? |
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
done |
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 patch, left some comments below.
tools/src/main/java/org/apache/kafka/tools/ShareConsumerPerformance.java
Show resolved
Hide resolved
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
Outdated
Show resolved
Hide resolved
tools/src/main/java/org/apache/kafka/tools/ConsumerPerformance.java
Outdated
Show resolved
Hide resolved
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 this patch, some comments left
tools/src/main/java/org/apache/kafka/tools/ConsumerPerformance.java
Outdated
Show resolved
Hide resolved
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. 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/share_consumer_performance.py
Outdated
Show resolved
Hide resolved
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
tools/src/main/java/org/apache/kafka/tools/ConsumerPerformance.java
Outdated
Show resolved
Hide resolved
tools/src/main/java/org/apache/kafka/tools/ShareConsumerPerformance.java
Outdated
Show resolved
Hide resolved
tools/src/main/java/org/apache/kafka/tools/ShareConsumerPerformance.java
Outdated
Show resolved
Hide resolved
tools/src/main/java/org/apache/kafka/tools/ShareConsumerPerformance.java
Outdated
Show resolved
Hide resolved
tools/src/main/java/org/apache/kafka/tools/ShareConsumerPerformance.java
Show resolved
Hide resolved
…nd `messages` traces from the tools
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 update, some minor comments.
tools/src/main/java/org/apache/kafka/tools/ShareConsumerPerformance.java
Outdated
Show resolved
Hide resolved
tools/src/test/java/org/apache/kafka/tools/ConsumerPerformanceTest.java
Outdated
Show resolved
Hide resolved
tools/src/test/java/org/apache/kafka/tools/ShareConsumerPerformanceTest.java
Outdated
Show resolved
Hide resolved
tools/src/test/java/org/apache/kafka/tools/ShareConsumerPerformanceTest.java
Show resolved
Hide resolved
tools/src/test/java/org/apache/kafka/tools/ShareConsumerPerformanceTest.java
Outdated
Show resolved
Hide resolved
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.
LGTM, thanks!
tools/src/test/java/org/apache/kafka/tools/ConsumerPerformanceTest.java
Outdated
Show resolved
Hide resolved
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.
Apart from adding --command-property to both kafka-consumer-perf-test.sh and kafka-share-consumer-perf-test.sh, this looks good to me.
tools/src/main/java/org/apache/kafka/tools/ConsumerPerformance.java
Outdated
Show resolved
Hide resolved
What do you mean by e2e test? |
Since you have modified the e2e test file FYI: https://github.com/apache/kafka/blob/trunk/tests/README.md |
I will run them after andrew's comments are resolved |
@aheev Please resolve conflicts |
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. 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
|
# Conflicts: # tools/src/main/java/org/apache/kafka/tools/ConsumerPerformance.java
Yes, we should. I'll wait then. |
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 |
Thanks, Could you file a ticket to trace this issue? |
# Conflicts: # tools/src/main/java/org/apache/kafka/tools/ShareConsumerPerformance.java
|
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?
|
@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. |
@Yunyung ConsumerPerformanceService and ShareConsumerPerformanceService are used in
I think we need to fix our test suite first |
resolves https://issues.apache.org/jira/browse/KAFKA-19624
Reviewers: @brandboat, @AndrewJSchofield, @m1a2st