KAFKA-17645: KIP-1052: Enable warmup in producer performance test #19
Conversation
This commit should enable a test PR against the trunk
Now using an overallStats object with new constructor to contin the combined stats at end of test and a single printTotal method.
Refactor to remove declaration of overallStats and limit its scope to only the location where printTotal is called. Set Stats combine-constructor as package-private. Remove commented code.
- Fix callbacks after switch to steady state which were not being properly created. - Add constant for DEFAULT_REPORTING_INTERVAL_MS - Remove redundant producer.send calls - Add ternerary operator in state assignment - Add check for non-positive numRecords
- bytes are now properly accoutned for. - Refactor to use boolean isSteadyState over warmupRecords to track if a Stats object is of steady state
Stats objects now represent the whole test with 'stats' and the steady state portion of the test with 'steadyStateStats'. Remove merging of Stats latency arrays at end of test.
- Add boolean to stats objects to control if they should print or not - Add DEBUG print to show value of config.warmupRecords
- Use "long" for numRecords and warmupRecords instead of "Long" - Remove "* != null" for above - Remove unused "combiner" constructor
- Remove DEFAULT_REPORTING_INTERVAL_MS as a constructor parameter and move into Stats class; refactor Stats constructors to not use reportingInterval as a parameter - Remove DEBUG print statement
|
I was able to reproduce and fix the performance issue. I had failed to create the record callback for messages in steady state :( It should be fixed in the latest push, but I still think the logic in that conditional can be improved. re: checking warmup records before num records, I've added an exception for when numRecords <= 0. FYI, I will be out for the next week, but thanks so much for looking at this patch!! |
|
Thanks for the latest changes. Results look much better now: 2000000 records sent, 9998.350272 records/sec (0.00 MB/sec), 0.58 ms avg latency, 417.00 ms max latency, 417 ms 50th, 417 ms 95th, 417 ms 99th, 417 ms 99.9th.
1500000 steady state records sent, 9997.733847 records/sec (0.95 MB/sec), 0.52 ms avg latency, 9.00 ms max latency, 1 ms 50th, 1 ms 95th, 1 ms 99th, 2 ms 99.9th.There is still an issue with the overall throughput (0.00 MB/sec) though. I also see some checkstyle errors after rebasing.
No rush, take your time. |
|
Hi @matt-welch, there is one checkstyle error and you also probably need to rebase. |
|
@matt-welch sorry for the delay. Latest changes LGTM. Please, address the comments from @chia7712 and then I think we are good. Thanks. PS: Feel free to ping people for review when you do changes, as we don't get notifications for new commits. |
|
Hi @chia7712 @fvaleri . My latest commit refactors the use of a warmupStats object and a main stats object in favor of having one stats that covers the whole test and a second object, steadyStateStats, that will be used during steady state operation. |
|
@matt-welch thanks for your update. I will take a look later! |
|
Warning Rate limit exceeded@visz11 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 17 minutes and 33 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughThe changes introduce a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ProducerPerformance
participant Stats
participant PerfCallback
User->>ProducerPerformance: Start with --warmup-records=N
ProducerPerformance->>Stats: Initialize Stats (overall)
loop For each record
ProducerPerformance->>PerfCallback: Send record
PerfCallback->>Stats: Record metrics
alt Reached warmupRecords
ProducerPerformance->>Stats: Initialize steadyStateStats
end
PerfCallback->>Stats: If steadyStateActive, record in steadyStateStats
end
ProducerPerformance->>Stats: Print overall stats
alt steadyStateStats exists
ProducerPerformance->>Stats: Print steady-state stats
end
Poem
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@matt-welch sorry for late review. could you please sync the trunk? |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @arvi18, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
This pull request introduces a warmup phase to the Producer Performance test in Kafka. The goal is to get a more accurate analysis of steady-state performance by excluding the initial ramp-up period from the performance metrics. A new --warmup-records argument is added to specify the number of records to use for the warmup phase. The changes involve modifying the ProducerPerformance tool to incorporate the warmup logic and updating the Stats class to track both the overall and steady-state performance. Additionally, new unit tests are added to verify the functionality of the warmup feature.
Highlights
- Warmup Phase: Introduces a warmup phase to the Producer Performance test, controlled by the
--warmup-recordsparameter. - Steady-State Statistics: Calculates and prints separate statistics for the steady-state portion of the test, excluding the warmup period.
- New Argument: Adds a new
--warmup-recordsargument to theProducerPerformancetool to specify the number of warmup records. - Validation: Adds validation to ensure that
--warmup-recordsis less than--num-recordsand is a valid number.
Changelog
Click here to see the changelog
- tools/src/main/java/org/apache/kafka/tools/ProducerPerformance.java
- Added
--warmup-recordsargument to the argument parser (lines 345-353). - Modified the
startmethod to incorporate the warmup phase (lines 79-81). - Created a new
Statsobject (steadyStateStats) to track steady-state performance (lines 102-103). - Modified the
PerfCallbackconstructor to accept bothstatsandsteadyStateStats(line 105). - Modified the print statements to print both total and steady state stats (lines 128-130 and 140-142).
- Added
steadyStateStatsmember variable (line 166). - Added validation to ensure that
--warmup-recordsis less than--num-records(lines 557-559).
- Added
- tools/src/test/java/org/apache/kafka/tools/ProducerPerformanceTest.java
- Added new unit tests to verify the functionality of the warmup feature, including handling of fractional and string values for
--warmup-records(lines 496-520). - Added a test to verify the number of successful sends and the correct count of records in the
statsandsteadyStateStatsobjects (lines 522-543). - Added a test to verify that negative values for
--warmup-recordsare handled correctly (lines 545-565).
- Added new unit tests to verify the functionality of the warmup feature, including handling of fractional and string values for
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
A warmup's gentle hand,
Prepares the code, makes it grand,
Steady state's true test,
Performance at its best,
Insights gained across the land.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces a warmup period to the Producer Performance test, which will help in analyzing the steady-state performance of Kafka. The implementation looks good overall, with clear logic for handling the warmup records and printing separate statistics for the warmup and steady-state phases. The addition of unit tests is also a good practice.
Summary of Findings
- Stats object initialization: The Stats object is initialized with
config.numRecordswhich includes warmup records. It might be more appropriate to initialize it with the number of steady-state records to avoid confusion. - Steady state stats recording: The
steadyStateStatsobject records all latencies, bytes, and iterations, even during the warmup phase. This could skew the steady-state statistics. Consider only recording these metrics after the warmup phase is complete. - Negative warmup records: The code handles negative warmup records by setting
warmupRecordsto 0. Consider adding a check to prevent the user from setting a negative value forwarmupRecords.
Merge Readiness
The pull request is well-structured and includes unit tests. However, there are a few areas that could be improved to enhance the accuracy and clarity of the results. I recommend addressing the Stats object initialization and Steady state stats recording issues before merging. I am unable to directly approve this pull request, and users should have others review and approve this code before merging.
| if (steadyStateStats != null) { | ||
| this.steadyStateStats.record(latency, bytes, now); | ||
| this.steadyStateStats.iteration++; |
There was a problem hiding this comment.
The steadyStateStats object records all latencies, bytes, and iterations, even during the warmup phase. This could skew the steady-state statistics. Consider only recording these metrics after the warmup phase is complete, by adding a check to ensure that i >= config.warmupRecords.
| if (steadyStateStats != null) { | |
| this.steadyStateStats.record(latency, bytes, now); | |
| this.steadyStateStats.iteration++; | |
| if (steadyStateStats != null && i >= config.warmupRecords) { | |
| this.steadyStateStats.record(latency, bytes, now); | |
| this.steadyStateStats.iteration++; | |
| } |
| if (config.warmupRecords > 0) { | ||
| System.out.println("Warmup first " + config.warmupRecords + " records. Steady state results will print after the complete test summary."); | ||
| } | ||
| stats = new Stats(config.numRecords); |
There was a problem hiding this comment.
The stats object is initialized with config.numRecords, which includes the warmup records. Consider initializing it with config.numRecords - config.warmupRecords to represent only the steady-state records. This might provide a clearer representation of the steady-state statistics.
| stats = new Stats(config.numRecords); | |
| stats = new Stats(config.numRecords - config.warmupRecords); |
| if (warmupRecords >= numRecords) { | ||
| throw new ArgumentParserException("The value for --warmup-records must be strictly fewer than the number of records in the test, --num-records.", parser); | ||
| } |
There was a problem hiding this comment.
This check prevents warmupRecords from being greater than or equal to numRecords. Consider adding a check to prevent the user from setting a negative value for warmupRecords.
| if (warmupRecords >= numRecords) { | |
| throw new ArgumentParserException("The value for --warmup-records must be strictly fewer than the number of records in the test, --num-records.", parser); | |
| } | |
| if (warmupRecords < 0) { | |
| throw new ArgumentParserException("The value for --warmup-records must be non-negative.", parser); | |
| } | |
| if (warmupRecords >= numRecords) { |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
Hi @arvi18 |
Hi, We were testing our internal code reviewer bot on a few open source projects and unintentionally duplicated some PRs, including tagging users from prior threads. We overlooked the fact that this could be interpreted as spammy behavior. Thank you for your understanding, and sorry once again for the inconvenience. |
In order to better analyze steady-state performance of Kafka, this PR enables a warmup in the Producer Performance test. The warmup duration is specified as a number of records that are a subset of the total numRecords. If warmup records is greater than 0, the warmup is represented by a second Stats object which holds warmup results. Once warmup records have been exhausted, the test switches to using the existing Stats object. At end of test, if warmup was enabled, the summary of the whole test (warump + steady state) is printed followed by the summary of the steady-state portion of the test. If no warmup is used, summary prints don't change from existing behavior.
This contribution is an original work and is licensed to the Kafka project under the Apache license
Testing strategy comprises new Java unit tests added to ProducerPerformanceTests.java.
Committer Checklist (excluded from commit message)
Summary by CodeRabbit
New Features
--warmup-recordsoption to exclude an initial set of records from steady-state performance statistics in ProducerPerformance.Bug Fixes
--warmup-recordsargument to ensure only valid values are accepted.Tests
--warmup-recordsargument, including invalid input scenarios and correct statistics reporting.