junrao commented on code in PR #20844:
URL: https://github.com/apache/kafka/pull/20844#discussion_r2520449454
##########
clients/src/main/java/org/apache/kafka/common/config/AbstractConfig.java:
##########
@@ -316,18 +316,22 @@ public Map<String, Object>
valuesWithPrefixOverride(String prefix) {
String keyWithNoPrefix =
entry.getKey().substring(prefix.length());
ConfigDef.ConfigKey configKey =
definition.configKeys().get(keyWithNoPrefix);
if (configKey != null)
- result.put(keyWithNoPrefix,
definition.parseValue(configKey, entry.getValue(), true));
+ result.put(keyWithNoPrefix,
definition.parseValue(configKey, entry.getValue(), true,
allowDuplicateValueInList()));
else {
String keyWithNoSecondaryPrefix =
keyWithNoPrefix.substring(keyWithNoPrefix.indexOf('.') + 1);
configKey =
definition.configKeys().get(keyWithNoSecondaryPrefix);
if (configKey != null)
- result.put(keyWithNoPrefix,
definition.parseValue(configKey, entry.getValue(), true));
+ result.put(keyWithNoPrefix,
definition.parseValue(configKey, entry.getValue(), true,
allowDuplicateValueInList()));
}
}
}
return result;
}
+ protected boolean allowDuplicateValueInList() {
Review Comment:
Hmm, this is kind of weird. An AbstractConfig can have multiple configs of
List type, some of which allow duplicate and some others don't. How do we
support that?
I was thinking that we could change the implementation of `getList(String
key)`. We could get the validator for that key from the configDef and remove
duplicates if the validator doesn't allow duplicates.
##########
docs/upgrade.html:
##########
@@ -139,7 +139,9 @@ <h5><a id="upgrade_420_notable"
href="#upgrade_420_notable">Notable changes in 4
<ul>
<li>Null values are no longer accepted for most LIST-type
configurations, except those that explicitly
allow a null default value or where a null value has a
well-defined semantic meaning.</li>
- <li>Duplicate entries within the same list are no longer
permitted.</li>
+ <li>Most LIST-type configurations no longer accept
duplicate entries, except in cases where duplicates
Review Comment:
1. Should we change ValidList.ensureValid() to only log a warning for
duplicated values?
2. The following makes the generated "Valid Values" in html quite verbose.
It would be useful to improve that. For example, if empty is not allowed, we
probably don't need to say it. If empty is allowed, we could add "empty".
```
public String toString() {
return validString + (isEmptyAllowed ? " (empty config allowed)"
: " (empty not allowed)") +
(isNullAllowed ? " (null config allowed)" : " (null not
allowed)");
}
```
Also, "Valid Values" for `anyNonDuplicateValues` will show up as `[]`, which
is counter intuitive.
--
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]
