<<<That's how it was initially - here's your suggestion to have it return the extensions Yes, I can see how that might be confusing. To clarify, at https://github.com/apache/kafka/pull/5379/files/4e4ac78f03a200f46bfe31cf466cdf0dc17d2b30#diff-3de90af12234c40ce1647059e6725a55R81 I did suggest an improvement at that time: the constructor was invoking the method `void extensions(SaslExtensions)`, which was setting `this.extensions` after invoking `void validateExtension(SaslExtensions)`. The main motivation I had for that suggestion was to set `this.extensions` in the constructor instead of elsewhere. I did also suggest that `void validateExtensions(SaslExtensions)` instead return a value so we could write `this.extensions = validateExtensions(extensions)` in the constructor, and I can see now that part of the suggestion -- making `validateExtensions()` return a value -- was likely a mistake. I think the suggestion I am making now would fix that -- it keeps the assignment of `this.extensions` in the constructor where it belongs while keeping the semantics of the method ` validateExtensions()` simple (or `confirmValid()`, or whatever we call it if we change it -- I don't have a strong feeling about the name). The semantics of the method, as per the Javadoc, is to make sure the names and values conform to the specification's regular expression requirements and make sure the reserved "auth" name doesn't appear as an extension key. Currently as implemented `SaslExtensions validateExtensions(SaslExtensions)` complects 2 things: it validates what is passed in for legality, but it also takes on the responsibility of returning something that is valid: that second responsibility means having to deal with null to either return null or return an empty `SaslExtensions` instance. The method should be simple and do 1 thing only, which is to validate that what is given is legal. Then whoever calls the method, once they know what they have is legal, can determine what they want to do with it (i.e. maybe they want to keep null, or maybe they don't). I don't thi nk we want to say "null is illegal" in this case; I think it is legal for backwards compatibility. So I think this method should be `void validateExtension(SaslExtensions)` with null being considered legal, and then it is up to the constructor to store NO_SASL_EXTENSIONS in its `this.extensions` field in place of null.
Does that clarify it? [ Full content available at: https://github.com/apache/kafka/pull/5552 ] This message was relayed via gitbox.apache.org for [email protected]
