ahuang98 commented on code in PR #20136:
URL: https://github.com/apache/kafka/pull/20136#discussion_r2198338634
##########
core/src/main/scala/kafka/tools/StorageTool.scala:
##########
@@ -135,6 +135,15 @@ object StorageTool extends Logging {
featureNamesAndLevels(_).foreachEntry {
(k, v) => formatter.setFeatureLevel(k, v)
})
+ if (!config.quorumConfig.voters().isEmpty &&
Review Comment:
nit: why not use isPresent?
##########
core/src/main/scala/kafka/tools/StorageTool.scala:
##########
@@ -135,6 +135,15 @@ object StorageTool extends Logging {
featureNamesAndLevels(_).foreachEntry {
(k, v) => formatter.setFeatureLevel(k, v)
})
+ if (!config.quorumConfig.voters().isEmpty &&
+ (namespace.getString("initial_controllers") != null ||
namespace.getBoolean("standalone"))) {
Review Comment:
store value of initial_controllers and standalone in vars given that you may
need to reference them again later in the function?
##########
core/src/main/scala/kafka/tools/StorageTool.scala:
##########
@@ -135,6 +135,15 @@ object StorageTool extends Logging {
featureNamesAndLevels(_).foreachEntry {
(k, v) => formatter.setFeatureLevel(k, v)
})
+ if (!config.quorumConfig.voters().isEmpty &&
+ (namespace.getString("initial_controllers") != null ||
namespace.getBoolean("standalone"))) {
Review Comment:
or, thoughts on moving this check to be with the existing config validations
under L155?
##########
core/src/test/scala/unit/kafka/tools/StorageToolTest.scala:
##########
@@ -376,6 +376,8 @@ Found problem:
val availableDirs = Seq(TestUtils.tempDir())
val properties = new Properties()
properties.putAll(defaultStaticQuorumProperties)
+ properties.remove("controller.quorum.voters")
+ properties.put("controller.quorum.bootstrap.servers", "localhost:9093")
Review Comment:
hm, why do this instead of just using `defaultDynamicQuorumProperties`?
##########
docs/ops.html:
##########
@@ -4027,8 +4027,8 @@ <h5 class="anchor-heading"><a
id="static_versus_dynamic_kraft_quorums" class="an
When using a static quorum, the configuration file for each broker and
controller must specify
the IDs, hostnames, and ports of all controllers in
<code>controller.quorum.voters</code>.<p>
- In contrast, when using a dynamic quorum, you should set
- <code>controller.quorum.bootstrap.servers</code> instead. This configuration
key need not
+ In contrast, when using a dynamic quorum,
<code>controller.quorum.bootstrap.servers</code>
+ is set instead and <code>controller.quorum.voters</code> must not be set.
This configuration key need not
Review Comment:
btw, can you improve the config description as well? something like
[controller.quorum.voters](https://kafka.apache.org/documentation/#brokerconfigs_controller.quorum.voters)
Map of id/endpoint information for the set of static voters in a
comma-separated list of {id}@{host}:{port} entries. This is the old way of
defining membership for controller quorums and should NOT be set if using
dynamic quorums. See [Static versus Dynamic KRaft
Quorums](https://kafka.apache.org/documentation/#static_versus_dynamic_kraft_quorums)
for details.
For example: 1@localhost:9092,2@localhost:9093,3@localhost:9094
##########
core/src/test/scala/unit/kafka/tools/StorageToolTest.scala:
##########
@@ -384,6 +386,48 @@ Found problem:
() => runFormatCommand(stream, properties,
arguments.toSeq)).getMessage)
}
+ @Test
+ def testFormatWithStandaloneAndInitialControllersFailsWithVotersConfig():
Unit = {
Review Comment:
nit: break into two tests?
`testFormatWithStandaloneFailsWithStaticVotersConfig`
`testFormatWithInitialControllersFailsWithStaticVotersConfig`
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
