Hello Taras, > Do you think that something needs to be corrected in KIP to make it more understandable without PR? Do you have any advice? Ha - no. I just wanted to thank you for sharing the PR which helped me as a newbie.
> If I understood the question correctly: I was referring to (and did not understand) the removal of L141 in clients/src/main/java/org/apache/kafka/common/security/ssl/SslFactory.java in https://github.com/apache/kafka/pull/14203/files. But you are right - this can be discussed in the final PR. > There might be a better place for this public static method that creates the SslEngineFactory. Yes, I think this class should be moved to something like `server-common` module - but would like any of the committers to comment on this. Thanks, Ashwin On Fri, Sep 29, 2023 at 9:22 PM Taras Ledkov <tled...@apache.org> wrote: > Hi Ashwin, > > Thanks a lot for your review. > > > Thanks for the KIP and the PR (which helped me understand the change). > Do you think that something needs to be corrected in KIP to make it more > understandable without PR? Do you have any advice? > > > I could not understand one thing though - In > https://github.com/apache/kafka/pull/14203/, > > why did you have to remove the code which sets sslEngineFactoryConfig in > instantiateSslEngineFactory? > If I understood the question correctly: > I've refactored this method a bit. > SslFactory#instantiateSslEngineFactory was a private not-static method. > I've separated the code that really creates new instance of the > SslEngineFactory and place it into a public static method. There might be a > better place for this public static method that creates the > SslEngineFactory. I think we will discuss this at the final PR. Current PR > is just a demo / prototype to play. >
