Skip to content

KAFKA-13466:when kafka-console-producer.sh, delete unused config batch.size #11517

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

KAFKA-13466:when kafka-console-producer.sh, delete unused config batch.size #11517
showuon merged 1 commit intoapache:trunkfrom
peterwanner:trunk

Conversation

@peterwanner
Copy link
Copy Markdown

…consistent with official docs# Please enter the commit message for your changes. Lines starting

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)

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 PR. But I think the issue here, is the "batch-size" option is already not used everywhere in the code anymore. There is actually already another option max-partition-memory-bytes that will be fed into the "batch.size" config (and default to 16KB). Anyway, could you please help remove the unused "batch-size" option in the code? If you need help, just let me know. Thank you.

@peterwanner
Copy link
Copy Markdown
Author

@showuon ok ,and what shoud the lowest version like 2,x which the batch.size is not removed but still have the same error(default error)

@showuon
Copy link
Copy Markdown
Member

showuon commented Nov 23, 2021

what shoud the lowest version like 2,x which the batch.size is not removed but still have the same error(default error)

Sorry, I don't get your question. What is the error? What is default error? I thought the PR is to fix the "batch.size" default value inconsistency issue. I didn't see any error here.

Thanks.

@peterwanner
Copy link
Copy Markdown
Author

what shoud the lowest version like 2,x which the batch.size is not removed but still have the same error(default error)

Sorry, I don't get your question. What is the error? What is default error? I thought the PR is to fix the "batch.size" default value inconsistency issue. I didn't see any error here.

Thanks.

what shoud the lowest version like 2,x which the batch.size is not removed but still have the same error(default error)

Sorry, I don't get your question. What is the error? What is default error? I thought the PR is to fix the "batch.size" default value inconsistency issue. I didn't see any error here.

Thanks.

ok I got it , please review. Thanks so much

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 patch.
@peterwanner , could you please also update the PR title/description, and also the content in
KAFKA-13466 in JIRA? Thank you.

@peterwanner peterwanner changed the title KAFKA-13466:when kafka-console-producer.sh, batch.size defaults is In… KAFKA-13466:when kafka-console-producer.sh, delete unused config batch.size Nov 23, 2021
@peterwanner
Copy link
Copy Markdown
Author

ok, jira content and commit title both have changed now

@peterwanner
Copy link
Copy Markdown
Author

LGTM! Thanks for the patch. @peterwanner , could you please also update the PR title/description, and also the content in KAFKA-13466 in JIRA? Thank you.

@showuon Could you help merge ths PR? Thanks a lot.

@showuon
Copy link
Copy Markdown
Member

showuon commented Dec 6, 2021

@dajac , could you help review this PR? Thanks.

@showuon
Copy link
Copy Markdown
Member

showuon commented Feb 22, 2022

@peterwanner , could you please fix the conflicts, so that I can merge the PR? Thank you.

…h.size defaults # Please enter the commit message for your changes. Lines starting
@peterwanner
Copy link
Copy Markdown
Author

@showuon yes, I have resolve the conflicts, please merge the PR. Thanks a lot

@showuon
Copy link
Copy Markdown
Member

showuon commented Mar 3, 2022

@peterwanner , thanks for the update. I'll merge the PR after the jenkins build completed (and without breaking any tests). Thank you.

@showuon
Copy link
Copy Markdown
Member

showuon commented Mar 3, 2022

retrigger the jenkins build again.
https://ci-builds.apache.org/job/Kafka/job/kafka-pr/job/PR-11517/5/

@showuon
Copy link
Copy Markdown
Member

showuon commented Mar 4, 2022

Failed tests are unrelated and also failed in trunk branch.

    Build / ARM / org.apache.kafka.streams.integration.NamedTopologyIntegrationTest.shouldRemoveOneNamedTopologyWhileAnotherContinuesProcessing
    Build / JDK 8 and Scala 2.12 / org.apache.kafka.streams.integration.NamedTopologyIntegrationTest.shouldRemoveOneNamedTopologyWhileAnotherContinuesProcessing
    Build / JDK 11 and Scala 2.13 / org.apache.kafka.connect.integration.RebalanceSourceConnectorsIntegrationTest.testDeleteConnector
    Build / JDK 11 and Scala 2.13 / org.apache.kafka.connect.integration.RebalanceSourceConnectorsIntegrationTest.testDeleteConnector
    Build / JDK 11 and Scala 2.13 / kafka.server.DynamicBrokerReconfigurationTest.testThreadPoolResize()
    Build / JDK 11 and Scala 2.13 / kafka.server.DynamicBrokerReconfigurationTest.testThreadPoolResize()
    Build / JDK 11 and Scala 2.13 / org.apache.kafka.streams.integration.NamedTopologyIntegrationTest.shouldRemoveOneNamedTopologyWhileAnotherContinuesProcessing
    Build / JDK 17 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 / org.apache.kafka.streams.integration.NamedTopologyIntegrationTest.shouldRemoveOneNamedTopologyWhileAnotherContinuesProcessing
    Build / JDK 8 and Scala 2.12 / kafka.api.ConsumerBounceTest.testClose()
    Build / JDK 11 and Scala 2.13 / kafka.api.ConsumerBounceTest.testClose()
    Build / JDK 11 and Scala 2.13 / kafka.api.ConsumerBounceTest.testClose()

@showuon showuon merged commit ae76b9d into apache:trunk Mar 4, 2022
@showuon
Copy link
Copy Markdown
Member

showuon commented Mar 4, 2022

Thanks for the contribution, @peterwanner !

@ijuma
Copy link
Copy Markdown
Member

ijuma commented Mar 5, 2022

We should add a release note since this can cause some applications to break (it's now an error to pass this config vs a no op).

@showuon
Copy link
Copy Markdown
Member

showuon commented Mar 5, 2022

@ijuma , good call! Thanks for the reminder.
@peterwanner , could you submit another PR to add a note for v3.2.0 notable change section in doc/upgrade.html

@ijuma
Copy link
Copy Markdown
Member

ijuma commented Mar 6, 2022

Looks like there was a KIP for this too https://cwiki.apache.org/confluence/display/KAFKA/KIP-717%3A+Deprecate+batch-size+config+from+console+producer. We probably should vote it and update the status correctly. cc @kamalcph

@peterwanner
Copy link
Copy Markdown
Author

@showuon ok, i have submited another pr 11855. please have a look at, thank you.

@showuon
Copy link
Copy Markdown
Member

showuon commented Mar 7, 2022

OK, let's wait for the KIP discussion/vote passed, otherwise, we might need to revert this change. Thanks.

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.

5 participants