hachikuji commented on code in PR #12108:
URL: https://github.com/apache/kafka/pull/12108#discussion_r865377321
##########
metadata/src/main/java/org/apache/kafka/controller/ConfigurationControlManager.java:
##########
@@ -215,13 +215,17 @@ private void
incrementalAlterConfigResource(ConfigResource configResource,
}
List<String> newValueParts = getParts(newValue, key,
configResource);
if (opType == APPEND) {
- if (!newValueParts.contains(opValue)) {
- newValueParts.add(opValue);
+ for (String value: opValue.split(",")) {
+ if (!newValueParts.contains(value)) {
Review Comment:
Do we have an integration test which covers the case when the appended value
is already present? It kind of looks like the zk path doesn't handle that.
By the way, I found the naming a little confusing here. I think
`newValueParts` should be `currentValueParts` or something like that.
##########
core/src/test/scala/integration/kafka/api/PlaintextAdminIntegrationTest.scala:
##########
@@ -1751,13 +1777,32 @@ class PlaintextAdminIntegrationTest extends
BaseAdminIntegrationTest {
new AlterConfigOp(new ConfigEntry(LogConfig.CleanupPolicyProp,
LogConfig.Compact + "," + LogConfig.Delete), AlterConfigOp.OpType.SUBTRACT)
).asJavaCollection
- alterResult = client.incrementalAlterConfigs(Map(
+ alterConfigs = Map(
topic1Resource -> topic1AlterConfigs,
topic2Resource -> topic2AlterConfigs
- ).asJava)
+ )
+ alterResult = client.incrementalAlterConfigs(alterConfigs.asJava)
assertEquals(Set(topic1Resource, topic2Resource).asJava,
alterResult.values.keySet)
alterResult.all.get
+ if (isKRaftTest()) {
Review Comment:
I agree having some utilities would be nice. Rather than having something
really specific, maybe we just need a utility which waits until the brokers
have caught up to the controller end offset?
##########
metadata/src/main/java/org/apache/kafka/controller/ConfigurationControlManager.java:
##########
@@ -215,13 +215,17 @@ private void
incrementalAlterConfigResource(ConfigResource configResource,
}
List<String> newValueParts = getParts(newValue, key,
configResource);
if (opType == APPEND) {
- if (!newValueParts.contains(opValue)) {
- newValueParts.add(opValue);
+ for (String value: opValue.split(",")) {
Review Comment:
nit: conventionally we put a space before the colon
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
