You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "kevin-wu24 (via GitHub)" <gi...@apache.org> on 2025/09/17 17:28:41 UTC
[PR] KAFKA-19719: --no-intial-controllers should not assume kraft.version=1 [kafka]
kevin-wu24 opened a new pull request, #20551:
URL: https://github.com/apache/kafka/pull/20551
Just because a controller node sets `--no-initial-controllers` flag does not mean it is necessarily running kraft.version=1. The more precise meaning is that the controller node being formatted does not know what kraft version the cluster should be in, and therefore it is only safe to assume kraft.version=0. Only by setting one of `--standalone` or `--initial-controllers` is it known `kraft.version == 1`.
For example, it is a valid configuration to run a static quorum defined by `controller.quorum.voters` but have all the controllers format with `--no-initial-controllers`. In this case, specifying `--no-initial-controllers` alongside a metadata version that does not support `kraft.version=1` causes formatting to fail.
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-19719: --no-initial-controllers should not assume kraft.version=1 [kafka]
Posted by "mkansara23 (via GitHub)" <gi...@apache.org>.
mkansara23 commented on PR #20551:
URL: https://github.com/apache/kafka/pull/20551#issuecomment-3344833796
Hi @kevin-wu24 ,
I noticed 5 rename refactoring in your [recent commit](https://github.com/apache/kafka/commit/857b1e92cc5c75eb178e48613e5963755bc1b03b):
- Method Declaration testFormatWithoutInitialVotersFailsWithNewerKraftVersion -> - testFormatWithStaticQuorumFailsWithNewerKraftVersion.
- Single Variable Declaration noInitialControllersFlag -> hasDynamicQuorum.
- Method Declaration setNoInitialControllersFlag -> setHasDynamicQuorum.
- Method Declaration testFormatWithInitialVotersFailsWithOlderMetadataVersion -> - testFormatWithInitialVotersWithOlderMetadataVersion.
- Field Declaration noInitialControllersFlag -> hasDynamicQuorum.
How did you identify that these 5 elements were connected and should be renamed together?
- Did you find this task challenging in any way?
- Did you perform the renames manually or with tools?
- Would it be helpful if tools could detect and rename connected program elements together instead of one by one?
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-19719: --no-initial-controllers should not assume kraft.version=1 [kafka]
Posted by "FrankYang0529 (via GitHub)" <gi...@apache.org>.
FrankYang0529 commented on code in PR #20551:
URL: https://github.com/apache/kafka/pull/20551#discussion_r2365931943
##########
metadata/src/test/java/org/apache/kafka/metadata/storage/FormatterTest.java:
##########
@@ -571,31 +586,25 @@ public void testFormatWithoutNoInitialControllersFailsWithNewerKraftVersion() th
FormatterContext formatter1 = testEnv.newFormatter();
formatter1.formatter.setFeatureLevel("kraft.version", (short) 1);
formatter1.formatter.setUnstableFeatureVersionsEnabled(true);
- formatter1.formatter.setNoInitialControllersFlag(false);
assertFalse(formatter1.formatter.hasDynamicQuorum());
assertEquals(
- "Cannot set kraft.version to 1 unless one of the flags --standalone, --initial-controllers, or " +
- "--no-initial-controllers is used. For dynamic controllers support, try using one of " +
- "--standalone, --initial-controllers, or --no-initial-controllers.",
+ "Cannot set kraft.version to 1 unless controller.quorum.voters is empty and " +
+ "one of the flags --standalone, --initial-controllers, or -no-initial-controllers is used. " +
Review Comment:
ditto
##########
metadata/src/main/java/org/apache/kafka/metadata/storage/Formatter.java:
##########
@@ -348,20 +352,20 @@ private short effectiveKRaftFeatureLevel(Optional<Short> configuredKRaftVersionL
if (configuredKRaftVersionLevel.get() == 0) {
if (hasDynamicQuorum()) {
throw new FormatterException(
- "Cannot set kraft.version to " +
- configuredKRaftVersionLevel.get() +
- " if one of the flags --standalone, --initial-controllers, or --no-initial-controllers is used. " +
+ "Cannot set kraft.version to " + configuredKRaftVersionLevel.get() +
+ " if controller.quorum.voters is empty and one of the flags --standalone, " +
+ "--initial-controllers, or --no-initial-controllers is used. " +
"For dynamic controllers support, try removing the --feature flag for kraft.version."
);
}
} else {
if (!hasDynamicQuorum()) {
throw new FormatterException(
- "Cannot set kraft.version to " +
- configuredKRaftVersionLevel.get() +
- " unless one of the flags --standalone, --initial-controllers, or --no-initial-controllers is used. " +
- "For dynamic controllers support, try using one of --standalone, --initial-controllers, or " +
- "--no-initial-controllers."
+ "Cannot set kraft.version to " + configuredKRaftVersionLevel.get() +
+ " unless controller.quorum.voters is empty and one of the flags --standalone, " +
+ "--initial-controllers, or -no-initial-controllers is used. " +
Review Comment:
nit: `-no-initial-controllers` -> `--no-initial-controllers`.
##########
metadata/src/test/java/org/apache/kafka/metadata/storage/FormatterTest.java:
##########
@@ -478,9 +481,10 @@ public void testFormatWithoutInitialVotersFailsWithNewerKraftVersion() throws Ex
formatter1.formatter.setUnstableFeatureVersionsEnabled(true);
assertFalse(formatter1.formatter.hasDynamicQuorum());
assertEquals(
- "Cannot set kraft.version to 1 unless one of the flags --standalone, --initial-controllers, or " +
- "--no-initial-controllers is used. For dynamic controllers support, try using one of " +
- "--standalone, --initial-controllers, or --no-initial-controllers.",
+ "Cannot set kraft.version to 1 unless controller.quorum.voters is empty and " +
+ "one of the flags --standalone, --initial-controllers, or -no-initial-controllers is used. " +
Review Comment:
ditto
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-19719: --no-initial-controllers should not assume kraft.version=1 [kafka]
Posted by "kevin-wu24 (via GitHub)" <gi...@apache.org>.
kevin-wu24 commented on code in PR #20551:
URL: https://github.com/apache/kafka/pull/20551#discussion_r2375739260
##########
core/src/main/scala/kafka/tools/StorageTool.scala:
##########
@@ -163,16 +164,13 @@ object StorageTool extends Logging {
if (isStandalone) {
formatter.setInitialControllers(createStandaloneDynamicVoters(config))
Review Comment:
If you look at the dynamic quorum arguments, you cannot specify multiple at the same time:
```
val reconfigurableQuorumOptions = formatParser.addMutuallyExclusiveGroup()
```
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-19719: --no-initial-controllers should not assume kraft.version=1 [kafka]
Posted by "kevin-wu24 (via GitHub)" <gi...@apache.org>.
kevin-wu24 commented on code in PR #20551:
URL: https://github.com/apache/kafka/pull/20551#discussion_r2375784175
##########
test-common/test-common-runtime/src/main/java/org/apache/kafka/common/test/KafkaClusterTestKit.java:
##########
@@ -477,53 +487,43 @@ private void formatNode(
} else {
formatter.setMetadataLogDirectory(Optional.empty());
}
- if (nodes.bootstrapMetadata().featureLevel(KRaftVersion.FEATURE_NAME) > 0) {
- StringBuilder dynamicVotersBuilder = new StringBuilder();
- String prefix = "";
- if (standalone) {
- if (nodeId == TestKitDefaults.CONTROLLER_ID_OFFSET) {
- final var controllerNode = nodes.controllerNodes().get(nodeId);
- dynamicVotersBuilder.append(
- String.format(
- "%d@localhost:%d:%s",
- controllerNode.id(),
- socketFactoryManager.
- getOrCreatePortForListener(controllerNode.id(), controllerListenerName),
- controllerNode.metadataDirectoryId()
- )
- );
- formatter.setInitialControllers(DynamicVoters.parse(dynamicVotersBuilder.toString()));
- } else {
- formatter.setNoInitialControllersFlag(true);
- }
- } else if (initialVoterSet.isPresent()) {
- for (final var controllerNode : initialVoterSet.get().entrySet()) {
- final var voterId = controllerNode.getKey();
- final var voterDirectoryId = controllerNode.getValue();
- dynamicVotersBuilder.append(prefix);
- prefix = ",";
- dynamicVotersBuilder.append(
- String.format(
- "%d@localhost:%d:%s",
- voterId,
- socketFactoryManager.
- getOrCreatePortForListener(voterId, controllerListenerName),
- voterDirectoryId
- )
- );
- }
- formatter.setInitialControllers(DynamicVoters.parse(dynamicVotersBuilder.toString()));
- } else {
- for (TestKitNode controllerNode : nodes.controllerNodes().values()) {
- int port = socketFactoryManager.
- getOrCreatePortForListener(controllerNode.id(), controllerListenerName);
- dynamicVotersBuilder.append(prefix);
- prefix = ",";
- dynamicVotersBuilder.append(String.format("%d@localhost:%d:%s",
- controllerNode.id(), port, controllerNode.metadataDirectoryId()));
- }
+ StringBuilder dynamicVotersBuilder = new StringBuilder();
+ String prefix = "";
+ if (standalone) {
+ if (nodeId == TestKitDefaults.CONTROLLER_ID_OFFSET) {
Review Comment:
Yeah I'll change it just in case we ever change `BROKER_ID_OFFSET`.
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-19719: --no-intial-controllers should not assume kraft.version=1 [kafka]
Posted by "kevin-wu24 (via GitHub)" <gi...@apache.org>.
kevin-wu24 commented on PR #20551:
URL: https://github.com/apache/kafka/pull/20551#issuecomment-3307303414
Thanks for the reviews @chia7712 @brandboat @frankvicky. Hopefully I can clear up the intended semantics of `--no-initial-controllers` a bit.
> Should we consider deprecating --no-initial-controllers? It seems redundant and has no effect for either dynamic or static voter configurations.
No, we should not deprecate `--no-initial-controllers`, since it does have an effect for a dynamic voter configuration, but not for static voter configuration. Specifically, when the static config `controller.quorum.voters` is not set in `kafka.properties`, one of --standalone, --initial-controllers, or --no-initial-controllers, must be set. Otherwise, the storage tool fails. This shows clear intention from the user to format a dynamic quorum. Actually, when we specify `--no-initial-controllers`, but do not specify `controller.quorum.voters` it is safe to assume kraft.version > 0, so I will update the PR accordingly.
> Excuse me, what is the purpose of having both controller.quorum.voters and --no-initial-controllers when starting a static quorum cluster?
>
> I assumed --no-initial-controllers is used only in building a dynamic quorum cluster.
`--no-initial-controllers` is only used in building a dynamic quorum cluster, meaning that when a node specifies `controller.quorum.voters` the flag acts as a no-op. However, specifying the flag should never break formatting because the meaning of --no-initial-controllers is that "this node may not know the intended KRaft version of cluster, and will rely on KRaft to figure out the version". This flag breaking previously valid static configurations is a regression from https://github.com/apache/kafka/pull/17836.
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-19719: --no-initial-controllers should not assume kraft.version=1 [kafka]
Posted by "kevin-wu24 (via GitHub)" <gi...@apache.org>.
kevin-wu24 commented on code in PR #20551:
URL: https://github.com/apache/kafka/pull/20551#discussion_r2363090995
##########
core/src/main/scala/kafka/tools/StorageTool.scala:
##########
@@ -149,8 +149,9 @@ object StorageTool extends Logging {
})
val initialControllers = namespace.getString("initial_controllers")
val isStandalone = namespace.getBoolean("standalone")
- if (!config.quorumConfig.voters().isEmpty &&
- (Option(initialControllers).isDefined || isStandalone)) {
+ val staticVotersEmpty = config.quorumConfig.voters().isEmpty
+ formatter.setHasDynamicQuorum(staticVotersEmpty)
+ if (!staticVotersEmpty && (Option(initialControllers).isDefined || isStandalone)) {
Review Comment:
> Should we add another warning to remind users that --no-initial-controllers should not be used with static voters even though it is no-op?
I can add it to the flag's help output.
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-19719: --no-initial-controllers should not assume kraft.version=1 [kafka]
Posted by "jsancio (via GitHub)" <gi...@apache.org>.
jsancio commented on code in PR #20551:
URL: https://github.com/apache/kafka/pull/20551#discussion_r2372963345
##########
core/src/main/scala/kafka/tools/StorageTool.scala:
##########
@@ -344,12 +345,12 @@ object StorageTool extends Logging {
.action(storeTrue())
reconfigurableQuorumOptions.addArgument("--no-initial-controllers", "-N")
- .help("Used to initialize a server without a dynamic quorum topology. When setting this flag, " +
+ .help("Used to initialize a controller without specifying a dynamic quorum. When setting this flag, " +
"the controller.quorum.voters config should not be set, and controller.quorum.bootstrap.servers is set instead.")
.action(storeTrue())
reconfigurableQuorumOptions.addArgument("--initial-controllers", "-I")
- .help("Used to initialize a server with a specific dynamic quorum topology. The argument " +
+ .help("Used to initialize a controller with the specified dynamic quorum. The argument " +
Review Comment:
--standalone can only be used on the controller node but --no-initial-controllers and --initial-controllers can be used in the controller and broker nodes.
I.e. you can change the message back to using the word server instead of the word controller.
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-19719: --no-initial-controllers should not assume kraft.version=1 [kafka]
Posted by "kevin-wu24 (via GitHub)" <gi...@apache.org>.
kevin-wu24 commented on code in PR #20551:
URL: https://github.com/apache/kafka/pull/20551#discussion_r2369067587
##########
test-common/test-common-runtime/src/main/java/org/apache/kafka/common/test/KafkaClusterTestKit.java:
##########
@@ -477,53 +487,41 @@ private void formatNode(
} else {
formatter.setMetadataLogDirectory(Optional.empty());
}
- if (nodes.bootstrapMetadata().featureLevel(KRaftVersion.FEATURE_NAME) > 0) {
- StringBuilder dynamicVotersBuilder = new StringBuilder();
- String prefix = "";
- if (standalone) {
- if (nodeId == TestKitDefaults.CONTROLLER_ID_OFFSET) {
- final var controllerNode = nodes.controllerNodes().get(nodeId);
- dynamicVotersBuilder.append(
- String.format(
- "%d@localhost:%d:%s",
- controllerNode.id(),
- socketFactoryManager.
- getOrCreatePortForListener(controllerNode.id(), controllerListenerName),
- controllerNode.metadataDirectoryId()
- )
- );
- formatter.setInitialControllers(DynamicVoters.parse(dynamicVotersBuilder.toString()));
- } else {
- formatter.setNoInitialControllersFlag(true);
- }
- } else if (initialVoterSet.isPresent()) {
- for (final var controllerNode : initialVoterSet.get().entrySet()) {
- final var voterId = controllerNode.getKey();
- final var voterDirectoryId = controllerNode.getValue();
- dynamicVotersBuilder.append(prefix);
- prefix = ",";
- dynamicVotersBuilder.append(
- String.format(
- "%d@localhost:%d:%s",
- voterId,
- socketFactoryManager.
- getOrCreatePortForListener(voterId, controllerListenerName),
- voterDirectoryId
- )
- );
- }
- formatter.setInitialControllers(DynamicVoters.parse(dynamicVotersBuilder.toString()));
- } else {
- for (TestKitNode controllerNode : nodes.controllerNodes().values()) {
- int port = socketFactoryManager.
- getOrCreatePortForListener(controllerNode.id(), controllerListenerName);
- dynamicVotersBuilder.append(prefix);
- prefix = ",";
- dynamicVotersBuilder.append(String.format("%d@localhost:%d:%s",
- controllerNode.id(), port, controllerNode.metadataDirectoryId()));
- }
+ StringBuilder dynamicVotersBuilder = new StringBuilder();
+ String prefix = "";
+ if (standalone) {
+ if (nodeId == TestKitDefaults.CONTROLLER_ID_OFFSET) {
+ final var controllerNode = nodes.controllerNodes().get(nodeId);
+ dynamicVotersBuilder.append(
+ String.format(
+ "%d@localhost:%d:%s",
+ controllerNode.id(),
+ socketFactoryManager.
+ getOrCreatePortForListener(controllerNode.id(), controllerListenerName),
+ controllerNode.metadataDirectoryId()
+ )
+ );
formatter.setInitialControllers(DynamicVoters.parse(dynamicVotersBuilder.toString()));
}
+ formatter.setHasDynamicQuorum(true);
Review Comment:
Yeah, that's what the comment is saying. One node formats with --standalone and the others are formatting with --no-initial-controllers. For both arguments we need to call `formatter.setHasDynamicQuorum(true);` for this configuration.
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-19719: --no-initial-controllers should not assume kraft.version=1 [kafka]
Posted by "kevin-wu24 (via GitHub)" <gi...@apache.org>.
kevin-wu24 commented on PR #20551:
URL: https://github.com/apache/kafka/pull/20551#issuecomment-3312486245
Thanks for the review @chia7712.
> Should we backport this fix to 4.0 and 4.1?
Yeah, we should backport it since it affects both versions.
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-19719: --no-intial-controllers should not assume kraft.version=1 [kafka]
Posted by "kevin-wu24 (via GitHub)" <gi...@apache.org>.
kevin-wu24 commented on code in PR #20551:
URL: https://github.com/apache/kafka/pull/20551#discussion_r2360143814
##########
test-common/test-common-runtime/src/main/java/org/apache/kafka/common/test/KafkaClusterTestKit.java:
##########
@@ -182,18 +181,31 @@ private KafkaConfig createNodeConfig(TestKitNode node) throws IOException {
props.putIfAbsent(INTER_BROKER_LISTENER_NAME_CONFIG, brokerListenerName);
props.putIfAbsent(KRaftConfigs.CONTROLLER_LISTENER_NAMES_CONFIG, controllerListenerName);
- StringBuilder quorumVoterStringBuilder = new StringBuilder();
- String prefix = "";
- for (int nodeId : nodes.controllerNodes().keySet()) {
- quorumVoterStringBuilder.append(prefix).
- append(nodeId).
- append("@").
- append("localhost").
- append(":").
- append(socketFactoryManager.getOrCreatePortForListener(nodeId, controllerListenerName));
- prefix = ",";
+ if (!standalone && initialVoterSet.isEmpty()) {
+ StringBuilder quorumVoterStringBuilder = new StringBuilder();
+ String prefix = "";
+ for (int nodeId : nodes.controllerNodes().keySet()) {
+ quorumVoterStringBuilder.append(prefix).
+ append(nodeId).
+ append("@").
+ append("localhost").
+ append(":").
+ append(socketFactoryManager.getOrCreatePortForListener(nodeId, controllerListenerName));
+ prefix = ",";
+ }
+ props.put(QuorumConfig.QUORUM_VOTERS_CONFIG, quorumVoterStringBuilder.toString());
+ } else {
+ StringBuilder bootstrapServersStringBuilder = new StringBuilder();
+ String prefix = "";
+ for (int nodeId : nodes.controllerNodes().keySet()) {
+ bootstrapServersStringBuilder.append(prefix).
+ append("localhost").
+ append(":").
+ append(socketFactoryManager.getOrCreatePortForListener(nodeId, controllerListenerName));
+ prefix = ",";
+ }
+ props.put(QuorumConfig.QUORUM_BOOTSTRAP_SERVERS_CONFIG, bootstrapServersStringBuilder.toString());
Review Comment:
Update the integration test framework to better mirror a real cluster for dynamic quorum (i.e. voters config should not be specified and we should use bootstrap servers)
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-19719: --no-initial-controllers should not assume kraft.version=1 [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #20551:
URL: https://github.com/apache/kafka/pull/20551#discussion_r2362132473
##########
core/src/test/java/kafka/server/ReconfigurableQuorumIntegrationTest.java:
##########
@@ -84,9 +84,8 @@ public void testCreateAndDestroyReconfigurableCluster() throws Exception {
new TestKitNodes.Builder().
setNumBrokerNodes(1).
setNumControllerNodes(1).
- setFeature(KRaftVersion.FEATURE_NAME, KRaftVersion.KRAFT_VERSION_1.featureLevel()).
Review Comment:
the method `setFeature` is useless now. Could you please remove it?
##########
core/src/main/scala/kafka/tools/StorageTool.scala:
##########
@@ -149,8 +149,9 @@ object StorageTool extends Logging {
})
val initialControllers = namespace.getString("initial_controllers")
val isStandalone = namespace.getBoolean("standalone")
- if (!config.quorumConfig.voters().isEmpty &&
- (Option(initialControllers).isDefined || isStandalone)) {
+ val staticVotersEmpty = config.quorumConfig.voters().isEmpty
+ formatter.setHasDynamicQuorum(staticVotersEmpty)
+ if (!staticVotersEmpty && (Option(initialControllers).isDefined || isStandalone)) {
Review Comment:
I still feel it is weird that users configure `controller.quorum.voters` with `--no-initial-controllers`, but you are right that we should keep the compatibility.
Should we add another warning to remind users that `--no-initial-controllers` should not be used with static voters even though it is no-op?
##########
test-common/test-common-runtime/src/main/java/org/apache/kafka/common/test/KafkaClusterTestKit.java:
##########
@@ -477,53 +487,41 @@ private void formatNode(
} else {
formatter.setMetadataLogDirectory(Optional.empty());
}
- if (nodes.bootstrapMetadata().featureLevel(KRaftVersion.FEATURE_NAME) > 0) {
- StringBuilder dynamicVotersBuilder = new StringBuilder();
- String prefix = "";
- if (standalone) {
- if (nodeId == TestKitDefaults.CONTROLLER_ID_OFFSET) {
- final var controllerNode = nodes.controllerNodes().get(nodeId);
- dynamicVotersBuilder.append(
- String.format(
- "%d@localhost:%d:%s",
- controllerNode.id(),
- socketFactoryManager.
- getOrCreatePortForListener(controllerNode.id(), controllerListenerName),
- controllerNode.metadataDirectoryId()
- )
- );
- formatter.setInitialControllers(DynamicVoters.parse(dynamicVotersBuilder.toString()));
- } else {
- formatter.setNoInitialControllersFlag(true);
- }
- } else if (initialVoterSet.isPresent()) {
- for (final var controllerNode : initialVoterSet.get().entrySet()) {
- final var voterId = controllerNode.getKey();
- final var voterDirectoryId = controllerNode.getValue();
- dynamicVotersBuilder.append(prefix);
- prefix = ",";
- dynamicVotersBuilder.append(
- String.format(
- "%d@localhost:%d:%s",
- voterId,
- socketFactoryManager.
- getOrCreatePortForListener(voterId, controllerListenerName),
- voterDirectoryId
- )
- );
- }
- formatter.setInitialControllers(DynamicVoters.parse(dynamicVotersBuilder.toString()));
- } else {
- for (TestKitNode controllerNode : nodes.controllerNodes().values()) {
- int port = socketFactoryManager.
- getOrCreatePortForListener(controllerNode.id(), controllerListenerName);
- dynamicVotersBuilder.append(prefix);
- prefix = ",";
- dynamicVotersBuilder.append(String.format("%d@localhost:%d:%s",
- controllerNode.id(), port, controllerNode.metadataDirectoryId()));
- }
+ StringBuilder dynamicVotersBuilder = new StringBuilder();
+ String prefix = "";
+ if (standalone) {
+ if (nodeId == TestKitDefaults.CONTROLLER_ID_OFFSET) {
+ final var controllerNode = nodes.controllerNodes().get(nodeId);
+ dynamicVotersBuilder.append(
+ String.format(
+ "%d@localhost:%d:%s",
+ controllerNode.id(),
+ socketFactoryManager.
+ getOrCreatePortForListener(controllerNode.id(), controllerListenerName),
+ controllerNode.metadataDirectoryId()
+ )
+ );
formatter.setInitialControllers(DynamicVoters.parse(dynamicVotersBuilder.toString()));
}
+ formatter.setHasDynamicQuorum(true);
Review Comment:
Could you please add a clarifying comment to explain the actual case here? For example, the `nodeId != CONTROLLER_ID_OFFSET` means the controller nodes are using `--no-initial-controllers`.
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-19719: --no-intial-controllers should not assume kraft.version=1 [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on PR #20551:
URL: https://github.com/apache/kafka/pull/20551#issuecomment-3306687544
@kevin-wu24 thanks for this patch.
> it is a valid configuration to run a static quorum defined
by controller.quorum.voters but have all the controllers format with
--no-initial-controllers
Excuse me, what is the purpose of having both controller.quorum.voters and --no-initial-controllers when starting a static quorum cluster?
I assumed --no-initial-controllers is used only in building a dynamic quorum cluster.
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-19719: --no-initial-controllers should not assume kraft.version=1 [kafka]
Posted by "kevin-wu24 (via GitHub)" <gi...@apache.org>.
kevin-wu24 commented on code in PR #20551:
URL: https://github.com/apache/kafka/pull/20551#discussion_r2369434956
##########
metadata/src/main/java/org/apache/kafka/metadata/storage/Formatter.java:
##########
@@ -348,20 +352,20 @@ private short effectiveKRaftFeatureLevel(Optional<Short> configuredKRaftVersionL
if (configuredKRaftVersionLevel.get() == 0) {
if (hasDynamicQuorum()) {
throw new FormatterException(
- "Cannot set kraft.version to " +
- configuredKRaftVersionLevel.get() +
- " if one of the flags --standalone, --initial-controllers, or --no-initial-controllers is used. " +
+ "Cannot set kraft.version to " + configuredKRaftVersionLevel.get() +
+ " if controller.quorum.voters is empty and one of the flags --standalone, " +
+ "--initial-controllers, or --no-initial-controllers is used. " +
Review Comment:
Sorry for the confusion, but the original error message should be correct. If the static voters is not specified, --no-intial-controllers is set, and kraft.version is set to 0, the formatter should fail. The message says if controller.quorum.voters is empty AND one of the KIP-853 flags is set.
If static voters is specified, --no-initial-controllers i set, and kraft.version is set to 0, the tool does not fail. The `hasDynamicQuorum` boolean is true when static voters is empty and false otherwise.
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-19719: --no-initial-controllers should not assume kraft.version=1 [kafka]
Posted by "kevin-wu24 (via GitHub)" <gi...@apache.org>.
kevin-wu24 commented on code in PR #20551:
URL: https://github.com/apache/kafka/pull/20551#discussion_r2369434956
##########
metadata/src/main/java/org/apache/kafka/metadata/storage/Formatter.java:
##########
@@ -348,20 +352,20 @@ private short effectiveKRaftFeatureLevel(Optional<Short> configuredKRaftVersionL
if (configuredKRaftVersionLevel.get() == 0) {
if (hasDynamicQuorum()) {
throw new FormatterException(
- "Cannot set kraft.version to " +
- configuredKRaftVersionLevel.get() +
- " if one of the flags --standalone, --initial-controllers, or --no-initial-controllers is used. " +
+ "Cannot set kraft.version to " + configuredKRaftVersionLevel.get() +
+ " if controller.quorum.voters is empty and one of the flags --standalone, " +
+ "--initial-controllers, or --no-initial-controllers is used. " +
Review Comment:
Sorry for the confusion, but the original error message should be correct. If the static voters is not specified, --no-intial-controllers is set, and kraft.version is set to 0, the formatter should fail. The message says if controller.quorum.voters is empty AND one of the KIP-853 flags is set.
If static voters is specified, --no-initial-controllers is set, and kraft.version is set to 0, the tool does not fail. The `hasDynamicQuorum` boolean is true when static voters is empty and false otherwise.
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-19719: --no-initial-controllers should not assume kraft.version=1 [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on PR #20551:
URL: https://github.com/apache/kafka/pull/20551#issuecomment-3315478668
I send a message to 4.0.1 RC thread (https://lists.apache.org/thread/4gsgcdgybyrnwg3hodnlyxg33ckj30wy). wait for the response from @clolov
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-19719: --no-intial-controllers should not assume kraft.version=1 [kafka]
Posted by "frankvicky (via GitHub)" <gi...@apache.org>.
frankvicky commented on PR #20551:
URL: https://github.com/apache/kafka/pull/20551#issuecomment-3306285422
+1 to @brandboat
After this patch, it seems that the `--no-initial-controller` flag doesn't provide too much value.
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-19719: --no-initial-controllers should not assume kraft.version=1 [kafka]
Posted by "jsancio (via GitHub)" <gi...@apache.org>.
jsancio merged PR #20551:
URL: https://github.com/apache/kafka/pull/20551
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-19719: --no-initial-controllers should not assume kraft.version=1 [kafka]
Posted by "jsancio (via GitHub)" <gi...@apache.org>.
jsancio commented on code in PR #20551:
URL: https://github.com/apache/kafka/pull/20551#discussion_r2368926488
##########
core/src/main/scala/kafka/tools/StorageTool.scala:
##########
@@ -342,18 +339,21 @@ object StorageTool extends Logging {
val reconfigurableQuorumOptions = formatParser.addMutuallyExclusiveGroup()
reconfigurableQuorumOptions.addArgument("--standalone", "-s")
- .help("Used to initialize a controller as a single-node dynamic quorum.")
+ .help("Used to initialize a controller as a single-node dynamic quorum. When setting this flag, " +
+ "the controller.quorum.voters config must not be set, and controller.quorum.bootstrap.servers is set instead.")
.action(storeTrue())
reconfigurableQuorumOptions.addArgument("--no-initial-controllers", "-N")
- .help("Used to initialize a server without a dynamic quorum topology.")
+ .help("Used to initialize a server without a dynamic quorum topology. When setting this flag, " +
Review Comment:
Let's remove the word topology. It matches the phrase used for --standalone.
##########
core/src/main/scala/kafka/tools/StorageTool.scala:
##########
@@ -342,18 +339,21 @@ object StorageTool extends Logging {
val reconfigurableQuorumOptions = formatParser.addMutuallyExclusiveGroup()
reconfigurableQuorumOptions.addArgument("--standalone", "-s")
- .help("Used to initialize a controller as a single-node dynamic quorum.")
+ .help("Used to initialize a controller as a single-node dynamic quorum. When setting this flag, " +
+ "the controller.quorum.voters config must not be set, and controller.quorum.bootstrap.servers is set instead.")
.action(storeTrue())
reconfigurableQuorumOptions.addArgument("--no-initial-controllers", "-N")
- .help("Used to initialize a server without a dynamic quorum topology.")
+ .help("Used to initialize a server without a dynamic quorum topology. When setting this flag, " +
+ "the controller.quorum.voters config should not be set, and controller.quorum.bootstrap.servers is set instead.")
.action(storeTrue())
reconfigurableQuorumOptions.addArgument("--initial-controllers", "-I")
.help("Used to initialize a server with a specific dynamic quorum topology. The argument " +
Review Comment:
Same here. Do you mind removing the word topology?
##########
metadata/src/main/java/org/apache/kafka/metadata/storage/Formatter.java:
##########
@@ -217,8 +217,8 @@ public Formatter setInitialControllers(DynamicVoters initialControllers) {
return this;
}
- public Formatter setNoInitialControllersFlag(boolean noInitialControllersFlag) {
- this.noInitialControllersFlag = noInitialControllersFlag;
+ public Formatter setHasDynamicQuorum(boolean staticVotersEmpty) {
+ this.hasDynamicQuorum = staticVotersEmpty;
Review Comment:
The parameter name should be something like `hasDynamicQuorum`.
##########
metadata/src/test/java/org/apache/kafka/metadata/storage/FormatterTest.java:
##########
@@ -495,10 +499,26 @@ public void testFormatWithInitialVotersFailsWithOlderMetadataVersion() throws Ex
formatter1.formatter.setInitialControllers(DynamicVoters.
parse("1@localhost:8020:4znU-ou9Taa06bmEJxsjnw"));
formatter1.formatter.setUnstableFeatureVersionsEnabled(true);
- assertEquals("kraft.version could not be set to 1 because it depends on " +
- "metadata.version level 21",
- assertThrows(IllegalArgumentException.class,
- formatter1.formatter::run).getMessage());
+ formatter1.formatter.setHasDynamicQuorum(true);
+ formatter1.formatter.run();
+ assertEquals((short) 1, formatter1.formatter.featureLevels.getOrDefault("kraft.version", (short) 0));
+ }
+ }
+
+ @ParameterizedTest
+ @ValueSource(booleans = {false, true})
+ public void testFormatWithNoInitialControllersWithOlderMetadataVersion(boolean emptyStaticVoters) throws Exception {
+ try (TestEnv testEnv = new TestEnv(2)) {
+ FormatterContext formatter1 = testEnv.newFormatter();
+ formatter1.formatter.setReleaseVersion(MetadataVersion.IBP_3_8_IV0);
+ // This MV does not support kraft.version = 1
+ formatter1.formatter.setHasDynamicQuorum(emptyStaticVoters);
+ formatter1.formatter.run();
+ if (emptyStaticVoters) {
+ assertEquals((short) 1, formatter1.formatter.featureLevels.getOrDefault("kraft.version", (short) 0));
+ } else {
+ assertEquals((short) 0, formatter1.formatter.featureLevels.getOrDefault("kraft.version", (short) 1));
+ }
Review Comment:
Don't you want to differentiate between missing value and the default in the test? Why not use `featureLevels.get("kraft.version")`? This comment applies to a few places in this file.
##########
test-common/test-common-runtime/src/main/java/org/apache/kafka/common/test/KafkaClusterTestKit.java:
##########
@@ -182,18 +181,31 @@ private KafkaConfig createNodeConfig(TestKitNode node) throws IOException {
props.putIfAbsent(INTER_BROKER_LISTENER_NAME_CONFIG, brokerListenerName);
props.putIfAbsent(KRaftConfigs.CONTROLLER_LISTENER_NAMES_CONFIG, controllerListenerName);
- StringBuilder quorumVoterStringBuilder = new StringBuilder();
- String prefix = "";
- for (int nodeId : nodes.controllerNodes().keySet()) {
- quorumVoterStringBuilder.append(prefix).
- append(nodeId).
- append("@").
- append("localhost").
- append(":").
- append(socketFactoryManager.getOrCreatePortForListener(nodeId, controllerListenerName));
- prefix = ",";
+ if (!standalone && initialVoterSet.isEmpty()) {
+ StringBuilder quorumVoterStringBuilder = new StringBuilder();
+ String prefix = "";
+ for (int nodeId : nodes.controllerNodes().keySet()) {
+ quorumVoterStringBuilder.append(prefix).
+ append(nodeId).
+ append("@").
+ append("localhost").
+ append(":").
+ append(socketFactoryManager.getOrCreatePortForListener(nodeId, controllerListenerName));
+ prefix = ",";
+ }
+ props.put(QuorumConfig.QUORUM_VOTERS_CONFIG, quorumVoterStringBuilder.toString());
+ } else {
+ StringBuilder bootstrapServersStringBuilder = new StringBuilder();
+ String prefix = "";
+ for (int nodeId : nodes.controllerNodes().keySet()) {
+ bootstrapServersStringBuilder.append(prefix).
+ append("localhost").
+ append(":").
+ append(socketFactoryManager.getOrCreatePortForListener(nodeId, controllerListenerName));
+ prefix = ",";
+ }
+ props.put(QuorumConfig.QUORUM_BOOTSTRAP_SERVERS_CONFIG, bootstrapServersStringBuilder.toString());
Review Comment:
Thanks for fixing this.
##########
metadata/src/main/java/org/apache/kafka/metadata/storage/Formatter.java:
##########
@@ -348,20 +352,20 @@ private short effectiveKRaftFeatureLevel(Optional<Short> configuredKRaftVersionL
if (configuredKRaftVersionLevel.get() == 0) {
if (hasDynamicQuorum()) {
throw new FormatterException(
- "Cannot set kraft.version to " +
- configuredKRaftVersionLevel.get() +
- " if one of the flags --standalone, --initial-controllers, or --no-initial-controllers is used. " +
+ "Cannot set kraft.version to " + configuredKRaftVersionLevel.get() +
+ " if controller.quorum.voters is empty and one of the flags --standalone, " +
+ "--initial-controllers, or --no-initial-controllers is used. " +
Review Comment:
This this true? Doesn't the tool allow --no-initial-controller when controller.quorum.voters is specified?
##########
core/src/main/scala/kafka/tools/StorageTool.scala:
##########
@@ -163,16 +164,12 @@ object StorageTool extends Logging {
if (isStandalone) {
formatter.setInitialControllers(createStandaloneDynamicVoters(config))
}
- if (namespace.getBoolean("no_initial_controllers")) {
- formatter.setNoInitialControllersFlag(true)
- } else {
- if (config.processRoles.contains(ProcessRole.ControllerRole)) {
- if (config.quorumConfig.voters().isEmpty && formatter.initialVoters().isEmpty) {
+ if (!namespace.getBoolean("no_initial_controllers") &&
+ config.processRoles.contains(ProcessRole.ControllerRole) &&
+ staticVotersEmpty && formatter.initialVoters().isEmpty) {
Review Comment:
I would add a newline after `&&` and before `formatter.initialVoters().isEmpt`.
##########
metadata/src/test/java/org/apache/kafka/metadata/storage/FormatterTest.java:
##########
@@ -495,10 +499,26 @@ public void testFormatWithInitialVotersFailsWithOlderMetadataVersion() throws Ex
formatter1.formatter.setInitialControllers(DynamicVoters.
parse("1@localhost:8020:4znU-ou9Taa06bmEJxsjnw"));
formatter1.formatter.setUnstableFeatureVersionsEnabled(true);
- assertEquals("kraft.version could not be set to 1 because it depends on " +
- "metadata.version level 21",
- assertThrows(IllegalArgumentException.class,
- formatter1.formatter::run).getMessage());
+ formatter1.formatter.setHasDynamicQuorum(true);
+ formatter1.formatter.run();
+ assertEquals((short) 1, formatter1.formatter.featureLevels.getOrDefault("kraft.version", (short) 0));
+ }
+ }
+
+ @ParameterizedTest
+ @ValueSource(booleans = {false, true})
+ public void testFormatWithNoInitialControllersWithOlderMetadataVersion(boolean emptyStaticVoters) throws Exception {
+ try (TestEnv testEnv = new TestEnv(2)) {
+ FormatterContext formatter1 = testEnv.newFormatter();
+ formatter1.formatter.setReleaseVersion(MetadataVersion.IBP_3_8_IV0);
+ // This MV does not support kraft.version = 1
+ formatter1.formatter.setHasDynamicQuorum(emptyStaticVoters);
Review Comment:
This is a bit odd. Why not name the test parameter `hasDynamicQuorum`?
##########
test-common/test-common-runtime/src/main/java/org/apache/kafka/common/test/KafkaClusterTestKit.java:
##########
@@ -477,53 +487,41 @@ private void formatNode(
} else {
formatter.setMetadataLogDirectory(Optional.empty());
}
- if (nodes.bootstrapMetadata().featureLevel(KRaftVersion.FEATURE_NAME) > 0) {
- StringBuilder dynamicVotersBuilder = new StringBuilder();
- String prefix = "";
- if (standalone) {
- if (nodeId == TestKitDefaults.CONTROLLER_ID_OFFSET) {
- final var controllerNode = nodes.controllerNodes().get(nodeId);
- dynamicVotersBuilder.append(
- String.format(
- "%d@localhost:%d:%s",
- controllerNode.id(),
- socketFactoryManager.
- getOrCreatePortForListener(controllerNode.id(), controllerListenerName),
- controllerNode.metadataDirectoryId()
- )
- );
- formatter.setInitialControllers(DynamicVoters.parse(dynamicVotersBuilder.toString()));
- } else {
- formatter.setNoInitialControllersFlag(true);
- }
- } else if (initialVoterSet.isPresent()) {
- for (final var controllerNode : initialVoterSet.get().entrySet()) {
- final var voterId = controllerNode.getKey();
- final var voterDirectoryId = controllerNode.getValue();
- dynamicVotersBuilder.append(prefix);
- prefix = ",";
- dynamicVotersBuilder.append(
- String.format(
- "%d@localhost:%d:%s",
- voterId,
- socketFactoryManager.
- getOrCreatePortForListener(voterId, controllerListenerName),
- voterDirectoryId
- )
- );
- }
- formatter.setInitialControllers(DynamicVoters.parse(dynamicVotersBuilder.toString()));
- } else {
- for (TestKitNode controllerNode : nodes.controllerNodes().values()) {
- int port = socketFactoryManager.
- getOrCreatePortForListener(controllerNode.id(), controllerListenerName);
- dynamicVotersBuilder.append(prefix);
- prefix = ",";
- dynamicVotersBuilder.append(String.format("%d@localhost:%d:%s",
- controllerNode.id(), port, controllerNode.metadataDirectoryId()));
- }
+ StringBuilder dynamicVotersBuilder = new StringBuilder();
+ String prefix = "";
+ if (standalone) {
+ if (nodeId == TestKitDefaults.CONTROLLER_ID_OFFSET) {
+ final var controllerNode = nodes.controllerNodes().get(nodeId);
+ dynamicVotersBuilder.append(
+ String.format(
+ "%d@localhost:%d:%s",
+ controllerNode.id(),
+ socketFactoryManager.
+ getOrCreatePortForListener(controllerNode.id(), controllerListenerName),
+ controllerNode.metadataDirectoryId()
+ )
+ );
formatter.setInitialControllers(DynamicVoters.parse(dynamicVotersBuilder.toString()));
}
+ formatter.setHasDynamicQuorum(true);
Review Comment:
I don't under the added comment. This execute when standalone. Irrespective of the node id. Doesn't that mean --standalone and --no-initial-controllers?
##########
metadata/src/main/java/org/apache/kafka/metadata/storage/Formatter.java:
##########
@@ -348,20 +352,20 @@ private short effectiveKRaftFeatureLevel(Optional<Short> configuredKRaftVersionL
if (configuredKRaftVersionLevel.get() == 0) {
if (hasDynamicQuorum()) {
throw new FormatterException(
- "Cannot set kraft.version to " +
- configuredKRaftVersionLevel.get() +
- " if one of the flags --standalone, --initial-controllers, or --no-initial-controllers is used. " +
+ "Cannot set kraft.version to " + configuredKRaftVersionLevel.get() +
+ " if controller.quorum.voters is empty and one of the flags --standalone, " +
+ "--initial-controllers, or --no-initial-controllers is used. " +
"For dynamic controllers support, try removing the --feature flag for kraft.version."
);
}
} else {
if (!hasDynamicQuorum()) {
throw new FormatterException(
- "Cannot set kraft.version to " +
- configuredKRaftVersionLevel.get() +
- " unless one of the flags --standalone, --initial-controllers, or --no-initial-controllers is used. " +
- "For dynamic controllers support, try using one of --standalone, --initial-controllers, or " +
- "--no-initial-controllers."
+ "Cannot set kraft.version to " + configuredKRaftVersionLevel.get() +
+ " unless controller.quorum.voters is empty and one of the flags --standalone, " +
+ "--initial-controllers, or --no-initial-controllers is used. " +
Review Comment:
Extra space after --no-initial-controllers
##########
test-common/test-common-runtime/src/main/java/org/apache/kafka/common/test/TestKitNodes.java:
##########
@@ -93,11 +93,6 @@ public Builder setBootstrapMetadata(BootstrapMetadata bootstrapMetadata) {
return this;
}
- public Builder setFeature(String featureName, short level) {
- this.bootstrapMetadata = bootstrapMetadata.copyWithFeatureRecord(featureName, level);
- return this;
- }
Review Comment:
Thanks for finally removing this configuration.
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-19719: --no-initial-controllers should not assume kraft.version=1 [kafka]
Posted by "kevin-wu24 (via GitHub)" <gi...@apache.org>.
kevin-wu24 commented on code in PR #20551:
URL: https://github.com/apache/kafka/pull/20551#discussion_r2369040685
##########
metadata/src/main/java/org/apache/kafka/metadata/storage/Formatter.java:
##########
@@ -348,20 +352,20 @@ private short effectiveKRaftFeatureLevel(Optional<Short> configuredKRaftVersionL
if (configuredKRaftVersionLevel.get() == 0) {
if (hasDynamicQuorum()) {
throw new FormatterException(
- "Cannot set kraft.version to " +
- configuredKRaftVersionLevel.get() +
- " if one of the flags --standalone, --initial-controllers, or --no-initial-controllers is used. " +
+ "Cannot set kraft.version to " + configuredKRaftVersionLevel.get() +
+ " if controller.quorum.voters is empty and one of the flags --standalone, " +
+ "--initial-controllers, or --no-initial-controllers is used. " +
Review Comment:
Yeah, this needs to be fixed.
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-19719: --no-initial-controllers should not assume kraft.version=1 [kafka]
Posted by "kevin-wu24 (via GitHub)" <gi...@apache.org>.
kevin-wu24 commented on code in PR #20551:
URL: https://github.com/apache/kafka/pull/20551#discussion_r2368783876
##########
metadata/src/main/java/org/apache/kafka/metadata/storage/Formatter.java:
##########
@@ -348,20 +352,20 @@ private short effectiveKRaftFeatureLevel(Optional<Short> configuredKRaftVersionL
if (configuredKRaftVersionLevel.get() == 0) {
if (hasDynamicQuorum()) {
throw new FormatterException(
- "Cannot set kraft.version to " +
- configuredKRaftVersionLevel.get() +
- " if one of the flags --standalone, --initial-controllers, or --no-initial-controllers is used. " +
+ "Cannot set kraft.version to " + configuredKRaftVersionLevel.get() +
+ " if controller.quorum.voters is empty and one of the flags --standalone, " +
+ "--initial-controllers, or --no-initial-controllers is used. " +
"For dynamic controllers support, try removing the --feature flag for kraft.version."
);
}
} else {
if (!hasDynamicQuorum()) {
throw new FormatterException(
- "Cannot set kraft.version to " +
- configuredKRaftVersionLevel.get() +
- " unless one of the flags --standalone, --initial-controllers, or --no-initial-controllers is used. " +
- "For dynamic controllers support, try using one of --standalone, --initial-controllers, or " +
- "--no-initial-controllers."
+ "Cannot set kraft.version to " + configuredKRaftVersionLevel.get() +
+ " unless controller.quorum.voters is empty and one of the flags --standalone, " +
+ "--initial-controllers, or -no-initial-controllers is used. " +
Review Comment:
Sounds good, will fix
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-19719: --no-initial-controllers should not assume kraft.version=1 [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #20551:
URL: https://github.com/apache/kafka/pull/20551#discussion_r2374960673
##########
core/src/main/scala/kafka/tools/StorageTool.scala:
##########
@@ -163,16 +164,13 @@ object StorageTool extends Logging {
if (isStandalone) {
formatter.setInitialControllers(createStandaloneDynamicVoters(config))
Review Comment:
It seems that `--initial-controllers` becomes a no-op when `--standalone` is defined. Should we add a warning for this case?
##########
metadata/src/main/java/org/apache/kafka/metadata/storage/Formatter.java:
##########
@@ -348,20 +352,20 @@ private short effectiveKRaftFeatureLevel(Optional<Short> configuredKRaftVersionL
if (configuredKRaftVersionLevel.get() == 0) {
if (hasDynamicQuorum()) {
throw new FormatterException(
- "Cannot set kraft.version to " +
- configuredKRaftVersionLevel.get() +
- " if one of the flags --standalone, --initial-controllers, or --no-initial-controllers is used. " +
- "For dynamic controllers support, try removing the --feature flag for kraft.version."
+ "Cannot set kraft.version to " + configuredKRaftVersionLevel.get() +
Review Comment:
Would you mind using `0` instead of `configuredKRaftVersionLevel.get()`? for example:
```java
throw new FormatterException(
"Cannot set kraft.version to 0 if controller.quorum.voters is empty and one of the flags --standalone, " +
"--initial-controllers, or --no-initial-controllers is used. For dynamic controllers support, " +
"try removing the --feature flag for kraft.version."
);
```
##########
test-common/test-common-runtime/src/main/java/org/apache/kafka/common/test/KafkaClusterTestKit.java:
##########
@@ -477,53 +487,43 @@ private void formatNode(
} else {
formatter.setMetadataLogDirectory(Optional.empty());
}
- if (nodes.bootstrapMetadata().featureLevel(KRaftVersion.FEATURE_NAME) > 0) {
- StringBuilder dynamicVotersBuilder = new StringBuilder();
- String prefix = "";
- if (standalone) {
- if (nodeId == TestKitDefaults.CONTROLLER_ID_OFFSET) {
- final var controllerNode = nodes.controllerNodes().get(nodeId);
- dynamicVotersBuilder.append(
- String.format(
- "%d@localhost:%d:%s",
- controllerNode.id(),
- socketFactoryManager.
- getOrCreatePortForListener(controllerNode.id(), controllerListenerName),
- controllerNode.metadataDirectoryId()
- )
- );
- formatter.setInitialControllers(DynamicVoters.parse(dynamicVotersBuilder.toString()));
- } else {
- formatter.setNoInitialControllersFlag(true);
- }
- } else if (initialVoterSet.isPresent()) {
- for (final var controllerNode : initialVoterSet.get().entrySet()) {
- final var voterId = controllerNode.getKey();
- final var voterDirectoryId = controllerNode.getValue();
- dynamicVotersBuilder.append(prefix);
- prefix = ",";
- dynamicVotersBuilder.append(
- String.format(
- "%d@localhost:%d:%s",
- voterId,
- socketFactoryManager.
- getOrCreatePortForListener(voterId, controllerListenerName),
- voterDirectoryId
- )
- );
- }
- formatter.setInitialControllers(DynamicVoters.parse(dynamicVotersBuilder.toString()));
- } else {
- for (TestKitNode controllerNode : nodes.controllerNodes().values()) {
- int port = socketFactoryManager.
- getOrCreatePortForListener(controllerNode.id(), controllerListenerName);
- dynamicVotersBuilder.append(prefix);
- prefix = ",";
- dynamicVotersBuilder.append(String.format("%d@localhost:%d:%s",
- controllerNode.id(), port, controllerNode.metadataDirectoryId()));
- }
+ StringBuilder dynamicVotersBuilder = new StringBuilder();
+ String prefix = "";
+ if (standalone) {
+ if (nodeId == TestKitDefaults.CONTROLLER_ID_OFFSET) {
Review Comment:
The true port of the standalone controller is `TestKitDefaults.BROKER_ID_OFFSET + TestKitDefaults.CONTROLLER_ID_OFFSET`. We will likely not change the value of `BROKER_ID_OFFSET` in the future, but should we still use `BROKER_ID_OFFSET + CONTROLLER_ID_OFFSET` instead?
https://github.com/apache/kafka/blob/trunk/test-common/test-common-runtime/src/main/java/org/apache/kafka/common/test/TestKitNodes.java#L175
##########
metadata/src/test/java/org/apache/kafka/metadata/storage/FormatterTest.java:
##########
@@ -74,6 +74,7 @@ public class FormatterTest {
private static final int DEFAULT_NODE_ID = 1;
private static final Uuid DEFAULT_CLUSTER_ID = Uuid.fromString("b3dGE68sQQKzfk80C_aLZw");
+ private static final String KRAFT_VERSION = "kraft.version";
Review Comment:
Could it be replaced by `KRaftVersion.FEATURE_NAME`?
--
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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org