Skip to content
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

MINOR: replication factor is short #6508

Closed
wants to merge 1 commit into from

Conversation

norwood
Copy link
Contributor

@norwood norwood commented Mar 27, 2019

replication factor is short on brokers, but there are many places it is typed in. this fixes that.

Committer Checklist (excluded from commit message)

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

@norwood norwood force-pushed the replication-factor-is-short branch 2 times, most recently from 572ab15 to 069beb3 Compare March 29, 2019 16:07
@omkreddy
Copy link
Contributor

test failures are related to the PR changes.

@norwood norwood force-pushed the replication-factor-is-short branch from 069beb3 to 8f2d2b4 Compare March 31, 2019 02:06
@norwood
Copy link
Contributor Author

norwood commented Mar 31, 2019

how embarrassing.

@@ -938,7 +938,7 @@ object KafkaConfig {

/** ********* Replication configuration ***********/
.define(ControllerSocketTimeoutMsProp, INT, Defaults.ControllerSocketTimeoutMs, MEDIUM, ControllerSocketTimeoutMsDoc)
.define(DefaultReplicationFactorProp, INT, Defaults.DefaultReplicationFactor, MEDIUM, DefaultReplicationFactorDoc)
.define(DefaultReplicationFactorProp, SHORT, Defaults.DefaultReplicationFactor, MEDIUM, DefaultReplicationFactorDoc)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is an incompatible change which would require a KIP and a new major version.

@cmccabe
Copy link
Contributor

cmccabe commented Apr 2, 2019

Changing INT to SHORT in the publicly visible configs is an incompatible change which would require a KIP. And we'd have to do it in a new major version. Suggest removing that from the PR.

@norwood
Copy link
Contributor Author

norwood commented Apr 2, 2019

I guess its technically incompatible, however if you currently have this set to > Short.MAX_VALUE the brokers will start, but will not create (with defaults) valid topics. So the only usage affected doesn't even work properly.

IMO it seems like a bug fix to not let users get in to a bad state. but if you think it needs a KIP i'll write one up

@norwood norwood closed this Mar 9, 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
3 participants