Hi Magesh, Next round :)
1. It looks like MM2 will also support "admin." properties that affect AdminClients it creates and uses, which IIUC is the same prefix name to be used for managing the DLQ for sink connectors in this KIP. Doesn't that still leave room for conflict? I'm imagining a scenario like this: a Connect worker is configured to use the PrincipalConnectorClientConfigPolicy, someone tries to start an instance of an MM2 sink with "admin." properties beyond just "admin.sasl.jaas.config", and gets rejected because those properties are then interpreted by the worker as overrides for the AdminClient it uses to manage the DLQ. 2. (LGTM) 3. I'm convinced by this, as long as nobody else identifies a common use case that would involve a similar client config policy implementation that would be limited to a small set of whitelisted configs. For now keeping the PrincipalConnectorClientConfigPolicy sounds fine to me. Cheers, Chris On Tue, Apr 23, 2019 at 10:30 PM Magesh Nandakumar <mage...@confluent.io> wrote: > Hi all, > > I also have a draft implementation of the KIP > https://github.com/apache/kafka/pull/6624. I would still need to include > more tests and docs but I thought it would be useful to have this for the > KIP discussion. Looking forward to all of your valuable feedback. > > Thanks > Magesh > > On Tue, Apr 23, 2019 at 10:27 PM Magesh Nandakumar <mage...@confluent.io> > wrote: > > > Chrise, > > > > Thanks a lot for your feedback. I will address them in order of your > > questions/comments. > > > > 1. Thanks for bringing this to my attention about KIP-382. I had a closer > > look at the KIP and IIUC, the KIP allows `consumer.` prefix for > SourceConnector > > and producer. prefix for SinkConnector since those are additional > > connector properties to help resolve the Kafka cluster other than the one > > Connect framework knows about. Whereas, the proposal in KIP-458 applies > > producer policies for SinkConnectors and consumer policies > > SourceConnectors. So, from what I understand this new policy should work > > without any issues even for Mirror Maker 2.0. > > 2. I have updated the KIP to use a default value of null and use that to > > determine if we need to ignore overrides. > > 3. I would still prefer to keep the special > PrincipalConnectorClientConfigPolicy > > since that is one of the most common use cases one would choose to use > this > > feature. If we make it a general case, that would involve users requiring > > to add additional configuration and they might require well more than > just > > the list of configs but might also want some restriction on values. If > the > > concern is about users wanting principal and also other configs, it would > > still be possible by means of a custom implementation. As is, I would > > prefer to keep the proposal to be the same for this. Let me know your > > thoughts. > > > > Thanks, > > Magesh > > > > > > On Mon, Apr 22, 2019 at 3:44 PM Chris Egerton <chr...@confluent.io> > wrote: > > > >> Hi Magesh, > >> > >> This is an exciting KIP! I have a few questions/comments but overall I > >> like > >> the direction it's headed in and hope to see it included in the Connect > >> framework soon. > >> > >> 1. With the proposed "consumer.", "producer.", and "admin." prefixes, > how > >> will this interact with connectors such as the upcoming Mirror Maker 2.0 > >> (KIP-382) that already support properties with those prefixes? Would it > be > >> possible for a user to configure MM2 with those properties without them > >> being interpreted as Connect client overrides, without isolating MM2 > onto > >> its own cluster and using the IgnoreConnectorClientConfigPolicy policy? > >> 2. Is the IgnoreConnectorClientConfigPolicy class necessary? The default > >> for the connector.client.config.policy property could simply be null > >> instead of a new policy that, as far as I can tell, isn't an actual > policy > >> in that its validate(...) method is never invoked and instead > represents a > >> special case to the Connect framework that says "Drop all overrides and > >> never use me". > >> 3. The PrincipalConnectorClientConfigPolicy seems like a specific > instance > >> of a more general use case: allow exactly a small set of overrides and > no > >> others. Why not generalize here and create a policy that accepts a list > of > >> allowed overrides during configuration? > >> > >> Thanks again for the KIP. > >> > >> Cheers, > >> > >> Chris > >> > >> On Fri, Apr 19, 2019 at 2:53 PM Magesh Nandakumar <mage...@confluent.io > > > >> wrote: > >> > >> > Hi all, > >> > > >> > I've posted "KIP-458: Connector Client Config Override Policy", which > >> > allows users to override the connector client configurations based on > a > >> > policy defined by the administrator. > >> > > >> > The KIP can be found at > >> > > >> > > >> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-458%3A+Connector+Client+Config+Override+Policy > >> > . > >> > > >> > Looking forward for the discussion on the KIP and all of your > thoughts & > >> > feedback on this enhancement to Connect. > >> > > >> > Thanks, > >> > Magesh > >> > > >> > > >
