Some of these items are copied from my email in the KIP-416 discussion. Custom config is a term I invented to mean a config that does not exist in Kafka but is specified by a custom implementation of SslEngineFactory. The problem with custom configs (as I remember it) is that the list of configs is static in SslConfigs and config names are checked before SslFactory is created. ==> You must solve this problem because that is what killed KIP-383 and therefore is the sole reason why KIP-519 exists. ==> Your KIP does not have to implement the solution since it can be done in a future KIP, but your KIP must be compatible with the solution found. ==> A method to return the list of configs would help. This cannot be a static method in the interface since it cannot be overridden. ==> You could require a static method in the implementation class by convention, just like the constructor you require.
Please change it to say it takes the class name >> ssl.engine.factory.class - This configuration will take class of the below >> interface's type and will be used to create javax.net.ssl.SSLEngine object. I suggest to add an underscore to correspond to the dots and adjust the second constant name accordingly >> final static String SSL_ENGINEFACTORY_CLASS_CONFIG = >> "ssl.engine.factory.class"; >> final static String DEFAULT_SSL_ENGINEFACTORY_CLASS = >> "org.apache.kafka.common.security.ssl.DefaultSslEngineFactory"; Please specify in the KIP if the new config is reconfigurable or not. I agree your factory needs to create instances of SslEngine because of the client/server mode as used in Kafka. I don't understand why we need the SslContext in that factory. It is not even used by SslFactory. Last time I checked, it was only used by a junit which is not enough reason to pollute a public API. I was told SslFactory is a private API, so it is OK to remove sslContext() even in a minor release. The KIP must mention this change. Your time diagrams never call shouldBeRebuilt() I am not a fan of the method name shouldBeRebuilt()/shouldBeRebuiltFor() This was OK in a private implementation before but this will become a public API. Maybe the reason why this name does not fit the rest of Kafka's reconfiguration API is because your interface is replacing reconfigure with a call to the constructor but it is not a stated goal to keep the interface implementation immutable. This deserves some thought since I don't know what would be better yet. I would not document SslEngineFactoryInstantiator in the KIP since it is private to SslFactory. In fact, my preference would be to use a private method in SslFactory instead. In your Rejected Alternatives, you must justify why KIP-519 exists by explaining why KIP-383 should be rejected. I would prefer if your KIP was self-contained and did not refer to sample code when explaining something. Of course, you can link to your sample code once to say it exists. When I wrote KIP-383, I felt I needed a prototype before I could solidify the proposal. That's part of the reason why there was never a third iteration. -----Original Message----- From: Maulin Vasavada [mailto:maulin.vasav...@gmail.com] Sent: Tuesday, September 10, 2019 2:26 AM To: dev@kafka.apache.org Subject: Re: [DISCUSS] KIP-519: Make SSL context/engine configuration extensible Hi Clement/Rajini/Colin Please review our latest updates on the KIP and let me know your thoughts. Clement, please let me know if my understanding about the "custom configs" is correct based on what I wrote in the KIP. Thanks Maulin On Mon, Sep 9, 2019 at 3:28 PM Maulin Vasavada <maulin.vasav...@gmail.com> wrote: > Hi all > > Based on longer discussion on another KIP-486 we are opening this KIP-519 ( > https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=128650952 > ). > > Please help us review this and provide your suggestions. > > Thanks > Maulin >
