-
Notifications
You must be signed in to change notification settings - Fork 14k
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
KAFKA-18028: the effective kraft version of --no-initial-controllers should be 1 rather than 0 #17836
base: trunk
Are you sure you want to change the base?
Conversation
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.
thanks for addressing this issue!
@@ -143,6 +143,9 @@ object StorageTool extends Logging { | |||
if (namespace.getBoolean("standalone")) { | |||
formatter.setInitialControllers(createStandaloneDynamicVoters(config)) | |||
} | |||
if (namespace.getBoolean("no_initial_controllers")) { | |||
formatter.setNoInitialControllers(true) | |||
} |
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.
nit: can we change the if statement below into an else clause?
@@ -130,6 +132,7 @@ public class Formatter { | |||
* The initial KIP-853 voters. | |||
*/ | |||
private Optional<DynamicVoters> initialControllers = Optional.empty(); | |||
private boolean noInitialControllers = false; |
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.
nit: thoughts on calling this noInitialControllersFlag
? might make it more clear this is not just set when initial-controllers
is not supplied
4c03c5f
to
427381d
Compare
Hi @ahuang98, thanks for the review. I address all comments. |
@FrankYang0529 thank you! Last two requests, I was wondering if you could add the sequence of events/commands for reproducing the issue to the PR description, and also test your change manually (bring up kraft quorum locally)? |
Hi @ahuang98, thanks for the review. I will update description and test manually tomorrow. |
…should be 1 rather than 0 Signed-off-by: PoAn Yang <[email protected]>
Hi @ahuang98, I updated PR description and following is manually test results:
|
BTW, I found the adding controller command is different from the KIP-853. Create a followup Jira: https://issues.apache.org/jira/browse/KAFKA-18059. |
427381d
to
847ad3a
Compare
nice find and I have added feedback on the jira |
The
hasDynamicQuorum
only considers theinitialControllers
, which is configured when either--initial-controllers
or--standalone
is specified. It will result in the following unexpected behaviors:no-initial-controllers
configuration getkraft.version=0
--no-initial-controllers --feature kraft.version=1
To fix this, we should also allow
--no-initial-controllers
with--feature kraft.version=1
.Committer Checklist (excluded from commit message)