onobc commented on PR #18358: URL: https://github.com/apache/pulsar/pull/18358#issuecomment-1324149110
I went ahead and rebase/squash as the changes were splintered and this is somewhat of a different approach altogether. As for implementing the logic in the setters, I did it and it worked well. However, at that point it was clear to me that it did not belong in ClientConfigurationData as it was a bit overly complicated and opinionated on what it did w/ those properties. The CCD should just be a simple data POJO, nothing else. The builders are what currently doctor up the data and do the construction of the auth via its `authentication()` methods. This is where this logic belongs as well. The tradeoff is that there is a bit of duplication in the clients. This was present before this addition though. To factor it out would require that both client builders share some common base. Putting the logic in CCD was a function of the builders not having a common area for things like this. What this code does now is "If you call loadConf and pass in auth related properties in the map, they will be respected and the builder authentication() method will be called w/ those params". @nodece @eolivelli @cbornet PTAL -- 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]
