KAFKA-19875 Duplicated topic config prevents broker start

[m1a2st]

Add new test case for ConfigDef.ValidList.anyNonDuplicateValues(false, false) and clearify the update.html section

FYI: #20334 (comment)

[Yunyung]

Could you cover the case have one element is empty string in List like ("a", "", "a") and ("a", "", "c"). It’s possible, for example: bootstrap.servers=localhost:9091,,localhost:9092 (Two consecutive commas; this would result in List.of(localhost:9091, "", "localhost:9092")

And for completeness, should this kind of case be clarified in KIP?

[junrao]

@m1a2st : Thanks for the PR. A couple of comments.

[junrao]

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.

[m1a2st]

Using the allowDuplicateValueInList tag isn’t a good approach. I think we can change it to check whether key.validator is an instance of ValidList in parseValue. For the LogConfig.validate method, it should directly call the ConfigDef.parse method instead of using getList().

[junrao]

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.

[m1a2st]

Should we change ValidList.ensureValid() to only log a warning for duplicated values?

I think we can keep the exception-throwing behavior, but we now preprocess the user’s configuration to remove duplicate entries.

The following makes the generated "Valid Values" in html quite verbose.

I have updated the document