KAFKA-16181: Use incrementalAlterConfigs when updating broker configs by kafka-configs.sh #15304
Conversation
| @@ -405,7 +420,7 @@ object ConfigCommand extends Logging { | |||
| val alterOptions = new AlterConfigsOptions().timeoutMs(30000).validateOnly(false) | |||
| val alterLogLevelEntries = (configsToBeAdded.values.map(new AlterConfigOp(_, AlterConfigOp.OpType.SET)) | |||
| ++ configsToBeDeleted.map { k => new AlterConfigOp(new ConfigEntry(k, ""), AlterConfigOp.OpType.DELETE) } | |||
| ).asJavaCollection | |||
| ).asJavaCollection | |||
There was a problem hiding this comment.
this change maybe not needed!
core/src/test/scala/integration/kafka/admin/ConfigCommandIntegrationTest.scala
Outdated
Show resolved
Hide resolved
core/src/test/scala/integration/kafka/admin/ConfigCommandIntegrationTest.scala
Outdated
Show resolved
Hide resolved
| @@ -349,7 +349,7 @@ object ConfigCommand extends Logging { | |||
| } | |||
|
|
|||
| @nowarn("cat=deprecation") | |||
| private[admin] def alterConfig(adminClient: Admin, opts: ConfigCommandOptions): Unit = { | |||
| private[admin] def alterConfig(adminClient: Admin, opts: ConfigCommandOptions, useIncrementalAlterConfigs: Boolean): Unit = { | |||
There was a problem hiding this comment.
couldn't we set useIncrementalAlterConfigs to true as default specially as I can see that later we set it with true any way.
There was a problem hiding this comment.
Good catch, I added a case of updating broker configs with useIncrementalAlterConfigs=false, also add a case of updating topic configs which only support incrementalAlterConfigs.
| "--entity-type", "brokers", | ||
| "--entity-default") | ||
| ), true) | ||
| } |
There was a problem hiding this comment.
I can't see any tests for useIncrementalAlterConfigs set to false. Can we test this path as well if possible?
There was a problem hiding this comment.
This case doesn't make sense because we can't produce UnsupportedVersionException by setting inter.broker.protocol.version, so I'm waiting for help from other reviewers, and just leave it unchanged for now.
There was a problem hiding this comment.
@dengziming One option would be to mock the admin client. This would allow you to verify the behavior. Otherwise, we could also run the tool against an old version in a system test.
dengziming
force-pushed
the
KAFKA-16181
branch
from
efc3eaf
to
42aa586
Compare
dengziming
force-pushed
the
KAFKA-16181
branch
2 times, most recently
from
115573d
to
659cb08
Compare
|
@dajac Hello, this PR is ready for review except the upgrade docs, PTAL. |
More detailed description of your change
This PR implement KIP-1011, use incrementalAlterConfigs by default to alter broker configurations instead of the deprecated alterConfigs API, and fall back to deprecated alterConfigs API if the broker doesn't support incrementalAlterConfigs API.
Summary of testing strategy (including rationale)
UnsupportedVersionExceptionby setting inter.broker.protocol.versionCommitter Checklist (excluded from commit message)