kirktrue commented on code in PR #17340:
URL: https://github.com/apache/kafka/pull/17340#discussion_r1972681856
##########
tools/src/main/java/org/apache/kafka/tools/ProducerPerformance.java:
##########
@@ -148,6 +175,7 @@ KafkaProducer<byte[], byte[]>
createKafkaProducer(Properties props) {
Callback cb;
Review Comment:
I know this is unrelated to these changes, but why are `cb` and `stats`
declared at the instance level instead of just created inside `start()`?
##########
tools/src/main/java/org/apache/kafka/tools/ProducerPerformance.java:
##########
@@ -75,7 +76,13 @@ void start(String[] args) throws IOException {
// not thread-safe, do not share with other threads
SplittableRandom random = new SplittableRandom(0);
ProducerRecord<byte[], byte[]> record;
- stats = new Stats(config.numRecords, 5000);
+
+ System.out.println("DEBUG: config.warmupRecords=" +
config.warmupRecords + ", (config.warmupRecords > 0)=" + (config.warmupRecords
> 0));
Review Comment:
Just for clarity, this `DEBUG` is for development purposes and will be
removed before merging?
##########
tools/src/main/java/org/apache/kafka/tools/ProducerPerformance.java:
##########
@@ -51,6 +51,7 @@ public class ProducerPerformance {
public static final String DEFAULT_TRANSACTION_ID_PREFIX =
"performance-producer-";
public static final long DEFAULT_TRANSACTION_DURATION_MS = 3000L;
+ public static final int DEFAULT_REPORTING_INTERVAL_MS = 5000;
Review Comment:
It seems like this constant could be folded into the `Stats` class and
removed as a constructor parameter since it's never changed?
##########
tools/src/main/java/org/apache/kafka/tools/ProducerPerformance.java:
##########
@@ -94,7 +101,19 @@ void start(String[] args) throws IOException {
record = new ProducerRecord<>(config.topicName, payload);
long sendStartMs = System.currentTimeMillis();
- cb = new PerfCallback(sendStartMs, payload.length, stats);
Review Comment:
Any reason not to change this to:
```java
if (config.warmupRecords > 0 && i == config.warmupRecords) {
steadyStateStats = new Stats(config.numRecords - config.warmupRecords,
DEFAULT_REPORTING_INTERVAL_MS, true);
stats.steadyStateActive = true;
}
cb = new PerfCallback(sendStartMs, payload.length, stats, steadyStateStats);
```
True, `steadyStateStats` will be `null` up until the number of warmup
records has elapsed. As I read in `Stats`, passing in a `null` `Stats` object
doesn't hurt.
CMIIW.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
