You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2022/05/04 20:56:45 UTC
[GitHub] [kafka] hachikuji commented on a diff in pull request #12108: KAFKA-13862: Support Append/Subtract multiple config values in KRaft mode
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org