-
Notifications
You must be signed in to change notification settings - Fork 14.3k
KAFKA-17645: KIP-1052: Enable warmup in producer performance test #17340
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?
Changes from 15 commits
4cfb897
125a246
6dcaed8
46fcad9
df6a119
6dfbaa7
74f2f1c
baef288
50a7565
4dd891f
ce9ce6b
9be5385
fb8a4ca
440329b
a57554b
818d1c6
b0a6c20
b127087
a55a429
4ff4daa
7def5f4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
||
public static void main(String[] args) throws Exception { | ||
ProducerPerformance perf = new ProducerPerformance(); | ||
|
@@ -75,7 +76,10 @@ 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); | ||
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, DEFAULT_REPORTING_INTERVAL_MS); | ||
long startMs = System.currentTimeMillis(); | ||
|
||
ThroughputThrottler throttler = new ThroughputThrottler(config.throughput, startMs); | ||
|
@@ -94,7 +98,18 @@ void start(String[] args) throws IOException { | |
record = new ProducerRecord<>(config.topicName, payload); | ||
|
||
long sendStartMs = System.currentTimeMillis(); | ||
cb = new PerfCallback(sendStartMs, payload.length, stats); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any reason not to change this to: 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, CMIIW. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the suggestion! I think the refactored conditional is much easier to understand. Hopefully nobody minds the passing of null steadyStateStats. I've refactored this in my latest commit. |
||
if (config.warmupRecords > 0) { | ||
if (i < config.warmupRecords) { | ||
cb = new PerfCallback(sendStartMs, payload.length, stats); | ||
} else { | ||
if (i == config.warmupRecords) { | ||
steadyStateStats = new Stats(config.numRecords - config.warmupRecords, DEFAULT_REPORTING_INTERVAL_MS, config.warmupRecords > 0); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you please clarify what you mean? That conditional is required when warmup is enabled. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Apologies for the unclear comment. Please see the following code.
In this case, config.warmupRecords > 0 is always true. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Apologies if I'm still misunderstanding your query, but the conditional "config.warmupRecords > 0" will only evaluate to true when the argument '--warmup-records N' is invoked on the command line and N is greater than zero. The value for config.warmupRecords defaults to '0' otherwise. The if-clause on that line is the path used when a warmup is invoked. The else-clause will be used when no warmup has been requested. This else clause represents the same code path used in previous versions of the test with no warmup. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi @chia7712, to address your concern, I have temporarily added a DEBUG message that prints the value of config.warmupRecords and the value of the conditional (config.warmupRecords > 0) before the test. Some sample output of running the test with various values of warmupRecords follows. Note that, when negative values of warmupRecords are supplied on the CLI, we set config.warmupRecords to 0.
|
||
} | ||
cb = new PerfCallback(sendStartMs, payload.length, stats, steadyStateStats); | ||
} | ||
} else { | ||
cb = new PerfCallback(sendStartMs, payload.length, stats); | ||
} | ||
producer.send(record, cb); | ||
|
||
currentTransactionSize++; | ||
|
@@ -116,6 +131,10 @@ record = new ProducerRecord<>(config.topicName, payload); | |
|
||
/* print final results */ | ||
stats.printTotal(); | ||
/* print steady-state stats if relevant */ | ||
if (steadyStateStats != null) { | ||
steadyStateStats.printTotal(); | ||
} | ||
} else { | ||
// Make sure all messages are sent before printing out the stats and the metrics | ||
// We need to do this in a different branch for now since tests/kafkatest/sanity_checks/test_performance_services.py | ||
|
@@ -124,6 +143,10 @@ record = new ProducerRecord<>(config.topicName, payload); | |
|
||
/* print final results */ | ||
stats.printTotal(); | ||
/* print steady-state stats if relevant */ | ||
if (steadyStateStats != null) { | ||
steadyStateStats.printTotal(); | ||
} | ||
|
||
/* print out metrics */ | ||
ToolsUtils.printMetrics(producer.metrics()); | ||
|
@@ -148,6 +171,7 @@ KafkaProducer<byte[], byte[]> createKafkaProducer(Properties props) { | |
Callback cb; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I know this is unrelated to these changes, but why are There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Regarding cb and the stats objects, their scopes don't extend beyond the start() method and the callback which gets them passed in. I tested moving them and found no errors. Do you think that I should move their declaration in this patch? |
||
|
||
Stats stats; | ||
Stats steadyStateStats; | ||
|
||
static byte[] generateRandomPayload(Integer recordSize, List<byte[]> payloadByteList, byte[] payload, | ||
SplittableRandom random, boolean payloadMonotonic, long recordValue) { | ||
|
@@ -163,7 +187,7 @@ static byte[] generateRandomPayload(Integer recordSize, List<byte[]> payloadByte | |
} | ||
return payload; | ||
} | ||
|
||
static Properties readProps(List<String> producerProps, String producerConfig) throws IOException { | ||
Properties props = new Properties(); | ||
if (producerConfig != null) { | ||
|
@@ -326,6 +350,16 @@ static ArgumentParser argParser() { | |
"--producer.config, or --transactional-id but --transaction-duration-ms is not specified, " + | ||
"the default value will be 3000."); | ||
|
||
parser.addArgument("--warmup-records") | ||
.action(store()) | ||
.required(false) | ||
.type(Long.class) | ||
.metavar("WARMUP-RECORDS") | ||
.dest("warmupRecords") | ||
.setDefault(0L) | ||
.help("The number of records to treat as warmup; these initial records will not be included in steady-state statistics. " + | ||
"An additional summary line will be printed describing the steady-state statistics. (default: 0)."); | ||
|
||
return parser; | ||
} | ||
|
||
|
@@ -346,8 +380,13 @@ static class Stats { | |
private long windowTotalLatency; | ||
private long windowBytes; | ||
private long windowStart; | ||
private final boolean isSteadyState; | ||
|
||
public Stats(long numRecords, int reportingInterval) { | ||
this(numRecords, reportingInterval, false); | ||
} | ||
|
||
public Stats(long numRecords, int reportingInterval, boolean isSteadyState) { | ||
this.start = System.currentTimeMillis(); | ||
this.windowStart = System.currentTimeMillis(); | ||
this.iteration = 0; | ||
|
@@ -361,6 +400,26 @@ public Stats(long numRecords, int reportingInterval) { | |
this.windowBytes = 0; | ||
this.totalLatency = 0; | ||
this.reportingInterval = reportingInterval; | ||
this.isSteadyState = isSteadyState; | ||
} | ||
|
||
Stats(Stats first, Stats second) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you please remove this unused constructor? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
// create a Stats object that's the combination of two disjoint Stats objects | ||
this.start = Math.min(first.start, second.start); | ||
this.iteration = first.iteration + second.iteration; | ||
this.sampling = first.sampling; | ||
this.index = first.index() + second.index(); | ||
this.latencies = Arrays.copyOf(first.latencies, this.index); | ||
System.arraycopy(second.latencies, 0, this.latencies, first.index(), second.index()); | ||
this.maxLatency = Math.max(first.maxLatency, second.maxLatency); | ||
this.windowCount = first.windowCount + second.windowCount; | ||
this.windowMaxLatency = 0; | ||
this.windowTotalLatency = 0; | ||
this.totalLatency = first.totalLatency + second.totalLatency; | ||
this.reportingInterval = first.reportingInterval; | ||
this.isSteadyState = false; // false except in the steady-state case | ||
this.count = first.count + second.count; | ||
this.bytes = first.bytes + second.bytes; | ||
} | ||
|
||
public void record(int latency, int bytes, long time) { | ||
|
@@ -378,6 +437,9 @@ public void record(int latency, int bytes, long time) { | |
} | ||
/* maybe report the recent perf */ | ||
if (time - windowStart >= reportingInterval) { | ||
if (this.isSteadyState && count == windowCount) { | ||
System.out.println("Beginning steady state."); | ||
} | ||
printWindow(); | ||
newWindow(); | ||
} | ||
|
@@ -428,8 +490,9 @@ public void printTotal() { | |
double recsPerSec = 1000.0 * count / (double) elapsed; | ||
double mbPerSec = 1000.0 * this.bytes / (double) elapsed / (1024.0 * 1024.0); | ||
int[] percs = percentiles(this.latencies, index, 0.5, 0.95, 0.99, 0.999); | ||
System.out.printf("%d records sent, %.1f records/sec (%.2f MB/sec), %.2f ms avg latency, %.2f ms max latency, %d ms 50th, %d ms 95th, %d ms 99th, %d ms 99.9th.%n", | ||
System.out.printf("%d%s records sent, %f records/sec (%.2f MB/sec), %.2f ms avg latency, %.2f ms max latency, %d ms 50th, %d ms 95th, %d ms 99th, %d ms 99.9th.%n", | ||
count, | ||
this.isSteadyState ? " steady state" : "", | ||
recsPerSec, | ||
mbPerSec, | ||
totalLatency / (double) count, | ||
|
@@ -456,10 +519,19 @@ static final class PerfCallback implements Callback { | |
private final long start; | ||
private final int bytes; | ||
private final Stats stats; | ||
private final Stats steadyStateStats; | ||
|
||
public PerfCallback(long start, int bytes, Stats stats) { | ||
this.start = start; | ||
this.stats = stats; | ||
this.steadyStateStats = null; | ||
this.bytes = bytes; | ||
} | ||
|
||
public PerfCallback(long start, int bytes, Stats stats, Stats steadyStateStats) { | ||
this.start = start; | ||
this.stats = stats; | ||
this.steadyStateStats = steadyStateStats; | ||
this.bytes = bytes; | ||
} | ||
|
||
|
@@ -471,6 +543,10 @@ public void onCompletion(RecordMetadata metadata, Exception exception) { | |
if (exception == null) { | ||
this.stats.record(latency, bytes, now); | ||
this.stats.iteration++; | ||
if (steadyStateStats != null) { | ||
this.steadyStateStats.record(latency, bytes, now); | ||
this.steadyStateStats.iteration++; | ||
} | ||
} | ||
if (exception != null) | ||
exception.printStackTrace(); | ||
|
@@ -480,6 +556,7 @@ public void onCompletion(RecordMetadata metadata, Exception exception) { | |
static final class ConfigPostProcessor { | ||
final String topicName; | ||
final Long numRecords; | ||
final Long warmupRecords; | ||
final Integer recordSize; | ||
final double throughput; | ||
final boolean payloadMonotonic; | ||
|
@@ -493,6 +570,7 @@ public ConfigPostProcessor(ArgumentParser parser, String[] args) throws IOExcept | |
Namespace namespace = parser.parseArgs(args); | ||
this.topicName = namespace.getString("topic"); | ||
this.numRecords = namespace.getLong("numRecords"); | ||
this.warmupRecords = Math.max(namespace.getLong("warmupRecords"), 0); | ||
this.recordSize = namespace.getInt("recordSize"); | ||
this.throughput = namespace.getDouble("throughput"); | ||
this.payloadMonotonic = namespace.getBoolean("payloadMonotonic"); | ||
|
@@ -506,6 +584,9 @@ public ConfigPostProcessor(ArgumentParser parser, String[] args) throws IOExcept | |
if (numRecords != null && numRecords <= 0) { | ||
throw new ArgumentParserException("--num-records should be greater than zero", parser); | ||
} | ||
if (warmupRecords != null && warmupRecords >= numRecords) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
throw new ArgumentParserException("The value for --warmup-records must be strictly fewer than the number of records in the test, --num-records.", parser); | ||
} | ||
if (recordSize != null && recordSize <= 0) { | ||
throw new ArgumentParserException("--record-size should be greater than zero", parser); | ||
} | ||
|
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.
It seems like this constant could be folded into the
Stats
class and removed as a constructor parameter since it's never changed?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.
Earlier in the PR, Chia-Ping asked to declare this variable here, probably because it seems analogous to the DEFAULT_TRANSACTION_DURATION_MS. I think the intent here is to have this as a parameter than can be modified for more frequent reporting, but making it a final member of the Stats class seems roughly equivalent to me. @kirktrue, @chia7712 where do you think these constants should be declared?
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.
If we don't plan to modify the
reportingInterval
for now, it is fine to remove thereportingInterval
fromStats
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.
My latest commit moves DEFAULT_REPORTING_INTERVAL_MS into Stats.