You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2021/04/03 13:40:19 UTC
[GitHub] [kafka] lamberken opened a new pull request #10469: KAFKA-12611: Fix using random payload in ProducerPerformance incorrectly
lamberken opened a new pull request #10469:
URL: https://github.com/apache/kafka/pull/10469
In ProducerPerformance, random payload always same. It has a great impact when use the compression.type option.
https://issues.apache.org/jira/browse/KAFKA-12611
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] lamberken commented on a change in pull request #10469: KAFKA-12611: Fix using random payload in ProducerPerformance incorrectly
Posted by GitBox <gi...@apache.org>.
lamberken commented on a change in pull request #10469:
URL: https://github.com/apache/kafka/pull/10469#discussion_r611055153
##########
File path: tools/src/main/java/org/apache/kafka/tools/ProducerPerformance.java
##########
@@ -111,13 +111,8 @@ public static void main(String[] args) throws Exception {
producer.initTransactions();
/* setup perf test */
- byte[] payload = null;
+ byte[] payload = new byte[recordSize];
Review comment:
Got, move it to inside loop.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] lamberken edited a comment on pull request #10469: KAFKA-12611: Fix using random payload in ProducerPerformance incorrectly
Posted by GitBox <gi...@apache.org>.
lamberken edited a comment on pull request #10469:
URL: https://github.com/apache/kafka/pull/10469#issuecomment-814922763
hi @showuon, your comment is very nice.
> please add tests for this change.
But, it seems to be no need to add unit test for `ProducerPerformance` class.
e.g assertEquals(<expected>, <actual>)?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] lamberken commented on pull request #10469: KAFKA-12611: Fix using random payload in ProducerPerformance incorrectly
Posted by GitBox <gi...@apache.org>.
lamberken commented on pull request #10469:
URL: https://github.com/apache/kafka/pull/10469#issuecomment-813000740
hi @dengziming, I'm not familiar with this jmh benchmark tool, I can show you the performance metrics, use following test script.
```
cd kafka_2.12-2.7.0
./bin/kafka-producer-perf-test.sh \
--topic demo_p30 \
--num-records 5000000 \
--record-size 512 \
--throughput 2000000 \
--print-metrics \
--producer-props \
bootstrap.servers=kafka-001:9092 \
batch.size=512000 \
linger.ms=100 \
buffer.memory=128000000 \
compression.type=zstd
```
### Before patch, the compression-rate is 0.005
### After patch, the compression-rate is 0.611 which is the real rate
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dengziming commented on pull request #10469: KAFKA-12611: Fix using random payload in ProducerPerformance incorrectly
Posted by GitBox <gi...@apache.org>.
dengziming commented on pull request #10469:
URL: https://github.com/apache/kafka/pull/10469#issuecomment-812951810
Nice catch, could you also run this jmh benchmark to show the perf regression.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] lamberken commented on a change in pull request #10469: KAFKA-12611: Fix using random payload in ProducerPerformance incorrectly
Posted by GitBox <gi...@apache.org>.
lamberken commented on a change in pull request #10469:
URL: https://github.com/apache/kafka/pull/10469#discussion_r610999261
##########
File path: tools/src/main/java/org/apache/kafka/tools/ProducerPerformance.java
##########
@@ -127,15 +122,22 @@ public static void main(String[] args) throws Exception {
int currentTransactionSize = 0;
long transactionStartTime = 0;
for (long i = 0; i < numRecords; i++) {
+
+ if (payloadFilePath != null) {
+ payload = payloadByteList.get(random.nextInt(payloadByteList.size()));
+ } else if (recordSize != null) {
+ payload = new byte[recordSize];
Review comment:
ok, you can test it locally.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon commented on a change in pull request #10469: KAFKA-12611: Fix using random payload in ProducerPerformance incorrectly
Posted by GitBox <gi...@apache.org>.
showuon commented on a change in pull request #10469:
URL: https://github.com/apache/kafka/pull/10469#discussion_r608461857
##########
File path: tools/src/main/java/org/apache/kafka/tools/ProducerPerformance.java
##########
@@ -127,12 +122,18 @@ public static void main(String[] args) throws Exception {
int currentTransactionSize = 0;
long transactionStartTime = 0;
for (long i = 0; i < numRecords; i++) {
+
+ if (recordSize != null) {
+ payload = new byte[recordSize];
+ for (int j = 0; j < payload.length; ++j)
+ payload[j] = (byte) (random.nextInt(26) + 65);
+ }
+
if (transactionsEnabled && currentTransactionSize == 0) {
producer.beginTransaction();
transactionStartTime = System.currentTimeMillis();
}
-
if (payloadFilePath != null) {
payload = payloadByteList.get(random.nextInt(payloadByteList.size()));
}
Review comment:
I think the random payload generation should be the `else` case here. Like this:
```java
if (payloadFilePath != null) {
payload = payloadByteList.get(random.nextInt(payloadByteList.size()));
} else if (recordSize != null) {
payload = new byte[recordSize];
....
} else {
throw new IllegalArgumentException("no payload File Path or record Size provided");
}
```
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] chia7712 merged pull request #10469: KAFKA-12611: Fix using random payload in ProducerPerformance incorrectly
Posted by GitBox <gi...@apache.org>.
chia7712 merged pull request #10469:
URL: https://github.com/apache/kafka/pull/10469
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] lamberken commented on pull request #10469: KAFKA-12611: Fix using random payload in ProducerPerformance incorrectly
Posted by GitBox <gi...@apache.org>.
lamberken commented on pull request #10469:
URL: https://github.com/apache/kafka/pull/10469#issuecomment-817649360
> LGTM. will merge to trunk if QA pass and there is no objection :)
Thanks.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] tombentley commented on a change in pull request #10469: KAFKA-12611: Fix using random payload in ProducerPerformance incorrectly
Posted by GitBox <gi...@apache.org>.
tombentley commented on a change in pull request #10469:
URL: https://github.com/apache/kafka/pull/10469#discussion_r610624115
##########
File path: tools/src/main/java/org/apache/kafka/tools/ProducerPerformance.java
##########
@@ -127,15 +122,22 @@ public static void main(String[] args) throws Exception {
int currentTransactionSize = 0;
long transactionStartTime = 0;
for (long i = 0; i < numRecords; i++) {
+
+ if (payloadFilePath != null) {
+ payload = payloadByteList.get(random.nextInt(payloadByteList.size()));
+ } else if (recordSize != null) {
+ payload = new byte[recordSize];
Review comment:
I'm suggesting to just move the `new byte[recordSize]` outside the loop. Obviously the point of the PR is to _populate_ it inside the loop. If the `new byte[recordSize]` is inside the loop then with, e.g. a 1M record size and large `--num-records` there will be more GC pressure (unless and until the JIT figures out it can reuse the same memory).
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] chia7712 commented on a change in pull request #10469: KAFKA-12611: Fix using random payload in ProducerPerformance incorrectly
Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #10469:
URL: https://github.com/apache/kafka/pull/10469#discussion_r611445541
##########
File path: tools/src/main/java/org/apache/kafka/tools/ProducerPerformance.java
##########
@@ -111,13 +111,8 @@ public static void main(String[] args) throws Exception {
producer.initTransactions();
/* setup perf test */
- byte[] payload = null;
+ byte[] payload = new byte[recordSize];
Review comment:
@lamberken thanks for updating code.
> But I don't agree it can cause serious gc.
You are right and it is NOT a bad thing to do this "small" optimization, right? :)
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] tombentley commented on a change in pull request #10469: KAFKA-12611: Fix using random payload in ProducerPerformance incorrectly
Posted by GitBox <gi...@apache.org>.
tombentley commented on a change in pull request #10469:
URL: https://github.com/apache/kafka/pull/10469#discussion_r610531203
##########
File path: tools/src/main/java/org/apache/kafka/tools/ProducerPerformance.java
##########
@@ -127,15 +122,22 @@ public static void main(String[] args) throws Exception {
int currentTransactionSize = 0;
long transactionStartTime = 0;
for (long i = 0; i < numRecords; i++) {
+
+ if (payloadFilePath != null) {
+ payload = payloadByteList.get(random.nextInt(payloadByteList.size()));
+ } else if (recordSize != null) {
+ payload = new byte[recordSize];
Review comment:
I guess by allocating a new `byte[]` for each record and with big enough `recordSize` we'd generate quite a lot of garbage, so it might be better to allocate `payload` once outside the loop and just fill the buffer here.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] lamberken commented on pull request #10469: KAFKA-12611: Fix using random payload in ProducerPerformance incorrectly
Posted by GitBox <gi...@apache.org>.
lamberken commented on pull request #10469:
URL: https://github.com/apache/kafka/pull/10469#issuecomment-813750487
hi @mageshn @C0urante @gharris1727 , please review when you get a chance.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] lamberken commented on a change in pull request #10469: KAFKA-12611: Fix using random payload in ProducerPerformance incorrectly
Posted by GitBox <gi...@apache.org>.
lamberken commented on a change in pull request #10469:
URL: https://github.com/apache/kafka/pull/10469#discussion_r608661424
##########
File path: tools/src/main/java/org/apache/kafka/tools/ProducerPerformance.java
##########
@@ -127,12 +122,18 @@ public static void main(String[] args) throws Exception {
int currentTransactionSize = 0;
long transactionStartTime = 0;
for (long i = 0; i < numRecords; i++) {
+
+ if (recordSize != null) {
+ payload = new byte[recordSize];
+ for (int j = 0; j < payload.length; ++j)
+ payload[j] = (byte) (random.nextInt(26) + 65);
+ }
+
if (transactionsEnabled && currentTransactionSize == 0) {
producer.beginTransaction();
transactionStartTime = System.currentTimeMillis();
}
-
if (payloadFilePath != null) {
payload = payloadByteList.get(random.nextInt(payloadByteList.size()));
}
Review comment:
👍
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] lamberken commented on a change in pull request #10469: KAFKA-12611: Fix using random payload in ProducerPerformance incorrectly
Posted by GitBox <gi...@apache.org>.
lamberken commented on a change in pull request #10469:
URL: https://github.com/apache/kafka/pull/10469#discussion_r610679955
##########
File path: tools/src/main/java/org/apache/kafka/tools/ProducerPerformance.java
##########
@@ -127,15 +122,22 @@ public static void main(String[] args) throws Exception {
int currentTransactionSize = 0;
long transactionStartTime = 0;
for (long i = 0; i < numRecords; i++) {
+
+ if (payloadFilePath != null) {
+ payload = payloadByteList.get(random.nextInt(payloadByteList.size()));
+ } else if (recordSize != null) {
+ payload = new byte[recordSize];
Review comment:
@tombentley thanks for your review, has updated the patch.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] lamberken edited a comment on pull request #10469: KAFKA-12611: Fix using random payload in ProducerPerformance incorrectly
Posted by GitBox <gi...@apache.org>.
lamberken edited a comment on pull request #10469:
URL: https://github.com/apache/kafka/pull/10469#issuecomment-814922763
hi @showuon, your comment is very nice.
> please add tests for this change.
But, it seems to be no need to add unit test for `ProducerPerformance` class.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] lamberken commented on a change in pull request #10469: KAFKA-12611: Fix using random payload in ProducerPerformance incorrectly
Posted by GitBox <gi...@apache.org>.
lamberken commented on a change in pull request #10469:
URL: https://github.com/apache/kafka/pull/10469#discussion_r611441937
##########
File path: tools/src/main/java/org/apache/kafka/tools/ProducerPerformance.java
##########
@@ -111,13 +111,8 @@ public static void main(String[] args) throws Exception {
producer.initTransactions();
/* setup perf test */
- byte[] payload = null;
+ byte[] payload = new byte[recordSize];
Review comment:
@chia7712 patch updated. But I don't agree it can cause serious gc.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] lamberken edited a comment on pull request #10469: KAFKA-12611: Fix using random payload in ProducerPerformance incorrectly
Posted by GitBox <gi...@apache.org>.
lamberken edited a comment on pull request #10469:
URL: https://github.com/apache/kafka/pull/10469#issuecomment-813008820
hi @ijuma @dengziming, please review again if you are free, thanks.
By the way, the failed unit tests seems not related to this PR.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] chia7712 commented on a change in pull request #10469: KAFKA-12611: Fix using random payload in ProducerPerformance incorrectly
Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #10469:
URL: https://github.com/apache/kafka/pull/10469#discussion_r611035999
##########
File path: tools/src/main/java/org/apache/kafka/tools/ProducerPerformance.java
##########
@@ -111,13 +111,8 @@ public static void main(String[] args) throws Exception {
producer.initTransactions();
/* setup perf test */
- byte[] payload = null;
+ byte[] payload = new byte[recordSize];
Review comment:
There is a null check for `recordSize` so is it (`new byte[recordSize]`) a potential NPE ?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] tombentley commented on a change in pull request #10469: KAFKA-12611: Fix using random payload in ProducerPerformance incorrectly
Posted by GitBox <gi...@apache.org>.
tombentley commented on a change in pull request #10469:
URL: https://github.com/apache/kafka/pull/10469#discussion_r610772613
##########
File path: tools/src/main/java/org/apache/kafka/tools/ProducerPerformance.java
##########
@@ -127,15 +122,22 @@ public static void main(String[] args) throws Exception {
int currentTransactionSize = 0;
long transactionStartTime = 0;
for (long i = 0; i < numRecords; i++) {
+
+ if (payloadFilePath != null) {
+ payload = payloadByteList.get(random.nextInt(payloadByteList.size()));
+ } else if (recordSize != null) {
+ payload = new byte[recordSize];
Review comment:
Well there _is_ a small difference in the number of young garbage collections, which is what we'd exepect, right? But the ygct is small enough that it makes no difference in a tool which is measuring times using `System.currentTimeMillis()`. That being the case perhaps sticking with the original version would be better, since it would allow you to factor out the payload generation into a separate method.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] lamberken commented on a change in pull request #10469: KAFKA-12611: Fix using random payload in ProducerPerformance incorrectly
Posted by GitBox <gi...@apache.org>.
lamberken commented on a change in pull request #10469:
URL: https://github.com/apache/kafka/pull/10469#discussion_r610999261
##########
File path: tools/src/main/java/org/apache/kafka/tools/ProducerPerformance.java
##########
@@ -127,15 +122,22 @@ public static void main(String[] args) throws Exception {
int currentTransactionSize = 0;
long transactionStartTime = 0;
for (long i = 0; i < numRecords; i++) {
+
+ if (payloadFilePath != null) {
+ payload = payloadByteList.get(random.nextInt(payloadByteList.size()));
+ } else if (recordSize != null) {
+ payload = new byte[recordSize];
Review comment:
ok, I think you can test it locally.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] lamberken commented on pull request #10469: KAFKA-12611: Fix using random payload in ProducerPerformance incorrectly
Posted by GitBox <gi...@apache.org>.
lamberken commented on pull request #10469:
URL: https://github.com/apache/kafka/pull/10469#issuecomment-814922763
hi @showuon, nice advice!
> please add tests for this change.
It seems to be no need to add unit test for `ProducerPerformance` class.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] chia7712 commented on a change in pull request #10469: KAFKA-12611: Fix using random payload in ProducerPerformance incorrectly
Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #10469:
URL: https://github.com/apache/kafka/pull/10469#discussion_r611320152
##########
File path: tools/src/main/java/org/apache/kafka/tools/ProducerPerformance.java
##########
@@ -111,13 +111,8 @@ public static void main(String[] args) throws Exception {
producer.initTransactions();
/* setup perf test */
- byte[] payload = null;
+ byte[] payload = new byte[recordSize];
Review comment:
@lamberken thanks for updating code. This change makes it create new array for each loop (as @tombentley mentioned). Could you avoid this performance regression? For example, we can initialize the `payload` before loop and then assign the value to each byte only if `payloadFilePath == null && recordSize != null`.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] lamberken commented on pull request #10469: KAFKA-12611: Fix using random payload in ProducerPerformance incorrectly
Posted by GitBox <gi...@apache.org>.
lamberken commented on pull request #10469:
URL: https://github.com/apache/kafka/pull/10469#issuecomment-813008820
By the way, the failed unit tests seems not related to this PR.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] lamberken commented on a change in pull request #10469: KAFKA-12611: Fix using random payload in ProducerPerformance incorrectly
Posted by GitBox <gi...@apache.org>.
lamberken commented on a change in pull request #10469:
URL: https://github.com/apache/kafka/pull/10469#discussion_r610562341
##########
File path: tools/src/main/java/org/apache/kafka/tools/ProducerPerformance.java
##########
@@ -127,15 +122,22 @@ public static void main(String[] args) throws Exception {
int currentTransactionSize = 0;
long transactionStartTime = 0;
for (long i = 0; i < numRecords; i++) {
+
+ if (payloadFilePath != null) {
+ payload = payloadByteList.get(random.nextInt(payloadByteList.size()));
+ } else if (recordSize != null) {
+ payload = new byte[recordSize];
Review comment:
> generate quite a lot of garbage
really? why it can generate a lot of garbage? you can put some gc log.
> allocate payload once outside the loop
the payload always same.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] lamberken commented on a change in pull request #10469: KAFKA-12611: Fix using random payload in ProducerPerformance incorrectly
Posted by GitBox <gi...@apache.org>.
lamberken commented on a change in pull request #10469:
URL: https://github.com/apache/kafka/pull/10469#discussion_r610672598
##########
File path: tools/src/main/java/org/apache/kafka/tools/ProducerPerformance.java
##########
@@ -127,15 +122,22 @@ public static void main(String[] args) throws Exception {
int currentTransactionSize = 0;
long transactionStartTime = 0;
for (long i = 0; i < numRecords; i++) {
+
+ if (payloadFilePath != null) {
+ payload = payloadByteList.get(random.nextInt(payloadByteList.size()));
+ } else if (recordSize != null) {
+ payload = new byte[recordSize];
Review comment:
@tombentley, the result are same.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] chia7712 commented on pull request #10469: KAFKA-12611: Fix using random payload in ProducerPerformance incorrectly
Posted by GitBox <gi...@apache.org>.
chia7712 commented on pull request #10469:
URL: https://github.com/apache/kafka/pull/10469#issuecomment-818475902
@lamberken thanks for your contribution
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org