Hi Magesh, Changes look good to me! Excited to see this happen, hope the KIP passes :)
Cheers, Chris On Fri, Apr 26, 2019 at 1:44 PM Magesh Nandakumar <mage...@confluent.io> wrote: > Hi Chris, > > I have updated the KIP to reflect the changes that we discussed for the > prefix. Thanks for all your inputs. > > Thanks, > Magesh > > On Thu, Apr 25, 2019 at 2:18 PM Chris Egerton <chr...@confluent.io> wrote: > > > Hi Magesh, > > > > Agreed that we should avoid `dlq.admin`. I also don't have a strong > opinion > > between `connector.` and `.override`, but I have a slight inclination > > toward `.override` since `connector.` feels a little redundant given that > > the whole configuration is for the connector and the use of "override" > may > > shed a little light on how the properties for these clients are computed > > and help make the learning curve a little gentler on new devs and users. > > > > Regardless, I think the larger issue of conflicts with existing > properties > > (both in MM2 and potentially other connectors) has been satisfactorily > > addressed, so I'm happy. > > > > Cheers, > > > > Chris > > > > On Wed, Apr 24, 2019 at 11:14 AM Magesh Nandakumar <mage...@confluent.io > > > > wrote: > > > > > HI Chrise, > > > > > > You are right about the "admin." prefix creating conflicts. Here are > few > > > options that I can think of > > > > > > 1. Use `dlq.admin` since admin client is used only for DLQ. But this > > might > > > not really be the case in the future. So, we should possibly drop this > > idea > > > :) > > > 2. Use `connector.producer`, `connector.consumer` and > `connector.admin` > > - > > > provides better context that its connector specific property > > > 3. Use `producer.override`, '`consumer.override` and `admin.override` > - > > > provides better clarity that these are overrides. > > > > > > I don't have a strong opinion in choosing between #2 and #3. Let me > > > know what you think. > > > > > > Thanks > > > Magesh > > > > > > On Wed, Apr 24, 2019 at 10:25 AM Chris Egerton <chr...@confluent.io> > > > wrote: > > > > > > > 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 > > > > > >> > > > > > > >> > > > > > > > > > > > > > > > > > > > > >
