junrao commented on code in PR #20844:
URL: https://github.com/apache/kafka/pull/20844#discussion_r2529205645
##########
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:
> except those that explicitly allow a null default value or where a null
value has a well-defined semantic meaning.
Null is only allowed when it's the default, right? There is no way to
configure a null value explicitly in a config file.
> However, if users configure duplicate entries, the internal deduplication
logic will still handle them.
For backward compatibility, if users configure duplicate entries when they
are not accepted, duplicate entries will be ignored and a warning will be
logged.
##########
clients/src/main/java/org/apache/kafka/common/config/ConfigDef.java:
##########
@@ -1070,8 +1081,10 @@ private void validateIndividualValues(String name,
List<Object> values) {
}
public String toString() {
- return validString + (isEmptyAllowed ? " (empty config allowed)" :
" (empty not allowed)") +
- (isNullAllowed ? " (null config allowed)" : " (null not
allowed)");
+ String base = validString.validStrings.isEmpty() ? "any
non-duplicate values" : validString.toString();
Review Comment:
The following configs don't have a validator, but it seems that they shoud.
BrokerSecurityConfigs:
ssl.cipher.suites
sasl.enabled.mechanisms
sasl.kerberos.principal.to.local.rules
sasl.oauthbearer.expected.audience
DefaultConfigPropertyFilter
config.properties.exclude
DefaultTopicFilter
topics
topics.exclude
##########
clients/src/test/java/org/apache/kafka/common/config/AbstractConfigTest.java:
##########
@@ -292,22 +297,23 @@ public void testConfiguredInstancesClosedOnFailure() {
try {
Map<String, String> props = new HashMap<>();
- String threeConsumerInterceptors =
MockConsumerInterceptor.class.getName() + ", "
- + MockConsumerInterceptor.class.getName() + ", "
+ String threeConsumerInterceptors =
CloseInterceptor.class.getName() + ", "
Review Comment:
Why are we changing this test? `threeConsumerInterceptors` is no longer
accurate since there are only two interceptors now.
--
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]
