Skip to content

MINOR: set batch-size option value into batch.size config in consoleProducer#11855

Merged
showuon merged 1 commit intoapache:trunkfrom
peterwanner:trunk
Mar 15, 2022
Merged

MINOR: set batch-size option value into batch.size config in consoleProducer#11855
showuon merged 1 commit intoapache:trunkfrom
peterwanner:trunk

Conversation

@peterwanner
Copy link
Copy Markdown

More detailed description of your change,
if necessary. The PR title and PR message become
the squashed commit message, so use a separate
comment to ping reviewers.

Summary of testing strategy (including rationale)
for the feature or bug fix. Unit and/or integration
tests are expected for any behaviour change and
system tests should be considered for larger changes.

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

@showuon
Copy link
Copy Markdown
Member

showuon commented Mar 9, 2022

@peterwanner , since this removing existing public option needs KIP discussion/voting, and the v3.2.0 already KIP freezed, I think we should revert the change we did in #11517. That means, we don't need the change in docs/upgrade.html now. Sorry for not reminding you during the previous PR!

So, in short, I need your help to submit a PR (or use this PR) to:

  1. revert the change in KAFKA-13466:when kafka-console-producer.sh, delete unused config batch.size  #11517 to bring the batchSizeOpt argument back
  2. I found we accidentally remove the line here, where user can set the max-partition-memory-bytes to change the batch.size in producer. Anyway, please bring it back.
  3. Fix the bug in batchSizeOpt, by feeding in to the producer batch.size config if batch-size option is set. So that user can set the batch.size via batch-size option or max-partition-memory-bytes option.
  4. In batch.size option description, please note that this value in this value will be replaced if max-partition-memory-bytes is also set.
  5. The result: user can still work well using batch.size option (if set), and now, the option will take effect in the producer. And most importantly, it won't break any existing script.
  6. For the default value in batch-size, we can ignore that. After all, we will (and should) deprecate it soon.

Does that make sense?
Thanks.

@showuon
Copy link
Copy Markdown
Member

showuon commented Mar 10, 2022

@peterwanner , since it's reaching the end of the release. Please let me know if you can't work on it before next weekend. Thank you.

@peterwanner
Copy link
Copy Markdown
Author

@peterwanner , since it's reaching the end of the release. Please let me know if you can't work on it before next weekend. Thank you.

@showuon ok I will finish in the afternoon, please wait for a seconds. Thank you so much for your reminder

@peterwanner peterwanner changed the title MINOR: Clarify producer batch.size in upgrade docs MINOR: bring producer batch.size back Mar 10, 2022
@peterwanner
Copy link
Copy Markdown
Author

peterwanner commented Mar 12, 2022

@showuon could you look at this pr? or as what you say, it's reaching the end of the release

Copy link
Copy Markdown
Member

@showuon showuon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@peterwanner , thanks for the ping. Left some comments. Thanks.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should make the default value the same the max-partition-memory-bytes, which is 16 * 1024, so that we can do this:
this opt will be replaced if max-partition-memory-bytes is also set

Comment thread core/src/main/scala/kafka/tools/ConsoleProducer.scala Outdated
@peterwanner peterwanner changed the title MINOR: bring producer batch.size back MINOR: fix default producer batch.size config Mar 12, 2022
@peterwanner
Copy link
Copy Markdown
Author

@showuon ok, maybe i mis-understanding what you say in the first comment. Best Thanks for your reminder.

Copy link
Copy Markdown
Member

@showuon showuon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@peterwanner , thanks for the update. Overall looks good to me. Left some minor comments.

Comment thread core/src/test/scala/unit/kafka/tools/ConsoleProducerTest.scala Outdated
Comment thread core/src/test/scala/unit/kafka/tools/ConsoleProducerTest.scala Outdated
Comment thread core/src/test/scala/unit/kafka/tools/ConsoleProducerTest.scala Outdated
Comment thread core/src/test/scala/unit/kafka/tools/ConsoleProducerTest.scala Outdated
Comment thread core/src/main/scala/kafka/tools/ConsoleProducer.scala Outdated
Comment thread core/src/main/scala/kafka/tools/ConsoleProducer.scala Outdated
Comment thread core/src/main/scala/kafka/tools/ConsoleProducer.scala Outdated
Copy link
Copy Markdown
Member

@showuon showuon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for the PR!

@showuon showuon changed the title MINOR: fix default producer batch.size config MINOR: set batch-size option value into batch.size config Mar 13, 2022
@showuon showuon changed the title MINOR: set batch-size option value into batch.size config MINOR: set batch-size option value into batch.size config in consoleProducer Mar 13, 2022
@showuon
Copy link
Copy Markdown
Member

showuon commented Mar 13, 2022

Failed tests are unrelated:

Build / JDK 8 and Scala 2.12 / kafka.api.ConsumerBounceTest.testClose()
Build / JDK 17 and Scala 2.13 / kafka.admin.LeaderElectionCommandTest.[1] Type=Raft, Name=testPathToJsonFile, Security=PLAINTEXT
Build / JDK 17 and Scala 2.13 / kafka.admin.LeaderElectionCommandTest.[1] Type=Raft, Name=testPreferredReplicaElection, Security=PLAINTEXT
Build / JDK 17 and Scala 2.13 / kafka.admin.LeaderElectionCommandTest.[1] Type=Raft, Name=testTopicPartition, Security=PLAINTEXT
Build / JDK 17 and Scala 2.13 / kafka.admin.LeaderElectionCommandTest.[1] Type=Raft, Name=testPathToJsonFile, Security=PLAINTEXT
Build / JDK 11 and Scala 2.13 / kafka.server.DynamicBrokerReconfigurationTest.testThreadPoolResize()
Build / JDK 11 and Scala 2.13 / kafka.server.DynamicBrokerReconfigurationTest.testThreadPoolResize()
Build / JDK 17 and Scala 2.13 / kafka.server.DynamicBrokerReconfigurationTest.testThreadPoolResize()
Build / JDK 17 and Scala 2.13 / kafka.server.DynamicBrokerReconfigurationTest.testThreadPoolResize()

@showuon
Copy link
Copy Markdown
Member

showuon commented Mar 13, 2022

Will wait for another day to merge to see if there are other people want to have another look. Thanks.

@showuon showuon merged commit e8a762e into apache:trunk Mar 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants