KAFKA-6891 fixed config constraints in KafkaConnect + refactoring

[oleg-smith]

No description provided.

[rhauch]

@oleg-smith Thanks for the PR.

This is really big change that I think is moving the code in a better direction, but because it touches so many different components this needs to be carefully thought out. I noticed that CommonClientConfigDefs has helper methods for created ConfigKey instances for the common configs, but there are none for the producer and consumer and admin config keys. That makes me think that this is either incomplete or not quite the right way to refactor.

Can I suggest that in the short term we just fix the incorrect validation constrain in the Connect codebase with a very simple and easy-to-review-and-merge PR? Then, the refactoring discussion could continue and any subsequent PR can just refactor and not change any behavior.

[oleg-smith]

https://issues.apache.org/jira/browse/KAFKA-6891?focusedCommentId=16469767&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-16469767

Actually, I was suggested to do this refactoring in the framework of this task.

Helper methods were created only for reusable configs, it is not necessary to create them for unique configs.

Is there any real issue except it touched a lot of classes?
@ewencp ?

[rhauch]

IMO, a large refactoring like this that also changes behavior makes it really difficult to see exactly what behavior is changed. And, because it affects so many different components, it's more likely the refactoring (not the bug fix) is somewhat contentious and may take a while for everyone to come to a resolution.

That's why I suggested:

1. first fixing the exact bug in a very low risk and uncontentious PR that we can review and merge quickly, and
2. then doing the refactoring in a separate PR that explicitly does not change any behavior (making it easier to identify potential bugs)

@oleg-smith be aware that code freeze for 2.1 is on Monday, so #1 would be far easier to resolve by then. I do realize that you've submitted this months ago, and it would have been better if the contributors and committers had reviewed it earlier. But everyone is busy, and sometimes big changes like this get put on the back burner. :-(

[ewencp]

I prefer small incremental changes, but don't actually have an issue w/ large code changes if they make significant maintainability improvements. Usually the issues with large refactors/diffs is that they come with minor improvements (e.g. things like fixing typos or formatting, that then also make backporting patches painful and isn't a big win for either users or maintainers). If we make sure we maintain consistent configs across the board and support things properly everywhere, that's a good win, which is why I mentioned the refactor in the ticket.

re: the current patch, what I had in mind (and probably should have linked to in the ticket when I mentioned this initially, sorry!) was being able to do something like this https://github.com/apache/kafka/blob/trunk/clients/src/main/java/org/apache/kafka/clients/producer/ProducerConfig.java#L333, which tracks back through https://github.com/apache/kafka/blob/trunk/clients/src/main/java/org/apache/kafka/common/config/ConfigDef.java#L439 and https://github.com/apache/kafka/blob/trunk/clients/src/main/java/org/apache/kafka/common/config/SaslConfigs.java#L137.

[oleg-smith]

@ewencp @rhauch Do you think it can be merged now?

[oleg-smith]

@ewencp @rhauch Do you think it can be merged now?