Randall, Thanks a lot for the suggestions. I have incorporated the comments in the KIP.
Thanks, Magesh On Mon, May 6, 2019 at 6:52 PM Randall Hauch <rha...@gmail.com> wrote: > Thanks, Magesh. I do have a few pretty minor suggestions. > > 1) Define a bit more clearly in the "Proposed Changes" whether the configs > passed to the validate method via the ConnectorClientConfigRequest object > have or do not have the prefixes. > 2) Specify more clearly in (or around) the table which is the default > policy. Currently the Ignore policy "Behavior" just mentions that it's the > current behavior, but I think it would help that it is described as the > default for the property. > > Otherwise, this looks good to me. > > Best regards, > > Randall > > On Mon, May 6, 2019 at 8:12 PM Magesh Nandakumar <mage...@confluent.io> > wrote: > > > Konstantine, > > > > Thanks a lot for your feedback on the KIP. I have incorporated the > feedback > > using generics for Class. I have also updated the KIP to handle the > default > > value per Randall's suggestion. Let me know if you have any questions. > > > > Thanks, > > Magesh > > > > > > On Mon, May 6, 2019 at 1:58 PM Konstantine Karantasis < > > konstant...@confluent.io> wrote: > > > > > Thanks for the KIP Magesh, it's quite useful towards the goals for more > > > general multi-tenancy in Connect. > > > > > > Couple of comments from me too: > > > > > > I think the fact that the default policy is 'null' (no implementation) > > > should be mentioned on the table next to the available implementations. > > > Currently the KIP says: 'In addition to the default implementation, > ..." > > > but this is not very accurate because there is no concrete default > > > implementation. Just special handling of 'null' in > > > 'connector.client.config.policy' > > > > > > Regarding passing the overrides to the connector 'configure' method, I > > feel > > > it wouldn't hurt to pass them, but I also agree that leaving this out > at > > > the moment is the safest option. > > > > > > Since the interfaces and classes are listed in the KIP, I'd like to > note > > > that Class is used as a raw type in field and return value > declarations. > > We > > > should use the generic type instead. > > > > > > Thanks for this improvement proposal! > > > Konstantine > > > > > > On Mon, May 6, 2019 at 11:11 AM Magesh Nandakumar < > mage...@confluent.io> > > > wrote: > > > > > > > Randall, > > > > > > > > I was wondering if you had any thoughts on the above alternatives to > > deal > > > > with a default policy. If it's possible, I would like to finalize > the > > > > discussions and start a vote. > > > > Let me know your thoughts. > > > > > > > > Thanks, > > > > Magesh > > > > > > > > On Tue, Apr 30, 2019 at 8:46 PM Magesh Nandakumar < > > mage...@confluent.io> > > > > wrote: > > > > > > > > > Randall, > > > > > > > > > > The approach to return the to override configs could possibly make > it > > > > > cumbersome to implement a custom policy. This is a new > configuration > > > and > > > > if > > > > > you don't explicitly set it the existing behavior remains as-is. > Like > > > > > Chris, I also preferred this approach for the sake of simplicity. > If > > > not > > > > > for the default `null` I would prefer to fall back to using > `Ignore` > > > > which > > > > > is a misnomer to the interface spec but still gets the job done via > > > > > instanceOf checks. The other options I could think of are as > below:- > > > > > > > > > > - have an enforcePolicy() method in the interface which by > default > > > > > returns true and the Ignore implementation could return false > > > > > - introduce another worker config > allow.connector.config.overrides > > > > > with a default value of false and the default policy can be None > > > > > > > > > > Let me know what you think. > > > > > > > > > > Thanks > > > > > Magesh > > > > > > > > > > On Tue, Apr 30, 2019 at 6:52 PM Randall Hauch <rha...@gmail.com> > > > wrote: > > > > > > > > > >> Thanks, Chris. I still think it's strange to have a non-policy, > > since > > > > >> there's now special behavior for when the policy is not specified. > > > > >> > > > > >> Perhaps the inability for a policy implementation to represent the > > > > >> existing > > > > >> behavior suggests that the policy interface isn't quite right. > Could > > > the > > > > >> policy's "validate" method take the overrides that were supplied > and > > > > >> return > > > > >> the overrides that should be passed to the connector, yet still > > > throwing > > > > >> an > > > > >> exception if any supplied overrides are not allowed. Then the > > > different > > > > >> policy implementations might be: > > > > >> > > > > >> - Ignore (default) - returns all supplied override properties > > > > >> - None - throws exception if any override properties are > > supplied; > > > > >> always returns empty map if no overrides are provided > > > > >> - Principal - throws exception if other override properties are > > > > >> provided, but returns an empty map (since no properties should > be > > > > >> passed to > > > > >> the connector) > > > > >> - All - returns all provided override properties > > > > >> > > > > >> All override properties defined on the connector configuration > would > > > be > > > > >> passed to the policy for validation, and assuming there's no error > > all > > > > of > > > > >> these overrides would be used in the producer/consumer/admin > client. > > > The > > > > >> result of the policy call, however, is used to determine which of > > > these > > > > >> overrides are passed to the connector. > > > > >> > > > > >> This approach means that all behaviors can be implemented through > a > > > > policy > > > > >> class, including the defaults. It also gives a bit more control to > > > > custom > > > > >> policies, should that be warranted. For example, validating the > > > provided > > > > >> client overrides but passing all such override properties to the > > > > >> connector, > > > > >> which as I stated earlier is something I think connectors likely > > don't > > > > >> look > > > > >> for. > > > > >> > > > > >> Thoughts? > > > > >> > > > > >> Randall > > > > >> > > > > >> On Tue, Apr 30, 2019 at 6:07 PM Chris Egerton < > chr...@confluent.io> > > > > >> wrote: > > > > >> > > > > >> > Randall, > > > > >> > > > > > >> > The special behavior for null was my suggestion. There is no > > > > >> implementation > > > > >> > of the proposed interface that causes client overrides to be > > > ignored, > > > > so > > > > >> > the original idea was to have a special implementation that > would > > be > > > > >> > checked for by the Connect framework (probably via the > instanceof > > > > >> operator) > > > > >> > and, if present, cause all would-be overrides to be ignored. > > > > >> > > > > > >> > I thought this may be confusing to people who may see that > > behavior > > > > and > > > > >> > wonder how to recreate it themselves, so I suggested leaving > that > > > > policy > > > > >> > out and replace it with a check to see if a policy was specified > > at > > > > all. > > > > >> > > > > > >> > Would be interested in your thoughts on this, especially if > > there's > > > an > > > > >> > alternative that hasn't been proposed yet. > > > > >> > > > > > >> > Cheers, > > > > >> > > > > > >> > Chris > > > > >> > > > > > >> > On Tue, Apr 30, 2019, 18:01 Randall Hauch <rha...@gmail.com> > > wrote: > > > > >> > > > > > >> > > On Tue, Apr 30, 2019 at 4:20 PM Magesh Nandakumar < > > > > >> mage...@confluent.io> > > > > >> > > wrote: > > > > >> > > > > > > >> > > > Randall, > > > > >> > > > > > > > >> > > > Thanks a lot for your feedback. > > > > >> > > > > > > > >> > > > You bring up an interesting point regarding the overrides > > being > > > > >> > available > > > > >> > > > to the connectors. Today everything that is specified in the > > > > config > > > > >> > while > > > > >> > > > creating is available for the connector. But this is a > > specific > > > > case > > > > >> > and > > > > >> > > we > > > > >> > > > could do either of the following > > > > >> > > > > > > > >> > > > > > > > >> > > > - don't pass any configs with these prefixes to the > > > > >> ConnectorConfig > > > > >> > > > instance that's passed in the startConnector > > > > >> > > > - allow policies as to whether the configurations with > the > > > > >> prefixes > > > > >> > > > should be made available to the connector or not. Should > > this > > > > >> also > > > > >> > > > define a > > > > >> > > > list of configurations? > > > > >> > > > > > > > >> > > > I personally prefer not passing the configs to Connector > since > > > > >> that's > > > > >> > > > simple, straight forward and don't see a reason for the > > > connector > > > > to > > > > >> > > access > > > > >> > > > those. > > > > >> > > > > > > > >> > > > > > > >> > > I agree that these override properties should be effectively > new > > > > >> > > properties, in which case I'd also prefer that they be removed > > > from > > > > >> the > > > > >> > > configuration before it is passed to the connector. Yes, it is > > > > >> *possible* > > > > >> > > that an existing connector happened to use connector config > > > > properties > > > > >> > with > > > > >> > > these prefixes, but it's seems pretty unlikely. > > > > >> > > > > > > >> > > I'd love to hear whether other people agree. > > > > >> > > > > > > >> > > > > > > >> > > > > > > > >> > > > For the second point, None - doesn't allow overrides and > the > > > > >> default > > > > >> > > > policy is null. We preserve backward compatibility when no > > > policy > > > > is > > > > >> > > > configured. Let me know if that's not clear in the KIP. > > > > >> > > > > > > > >> > > > > > > >> > > Why not have a default policy (rather than null) that > implements > > > the > > > > >> > > backward-compatible behavior? It seems strange to have null be > > the > > > > >> > default > > > > >> > > and for non-policy to allow anything. > > > > >> > > > > > > >> > > > > > > >> > > > > > > > >> > > > Thanks, > > > > >> > > > Magesh > > > > >> > > > > > > > >> > > > On Mon, Apr 29, 2019 at 4:07 PM Randall Hauch < > > rha...@gmail.com > > > > > > > > >> > wrote: > > > > >> > > > > > > > >> > > > > Per the proposal, a connector configuration can define one > > or > > > > more > > > > >> > > > > properties that begin with any of the three prefixes: > > > > >> > > > "producer.override.", > > > > >> > > > > "consumer.override.", and "admin.override.". The proposal > > > > states: > > > > >> > > > > > > > > >> > > > > Since the users can specify any of these policies, the > > > > connectors > > > > >> > > itself > > > > >> > > > > should not rely on these configurations to be available. > The > > > > >> > overrides > > > > >> > > > are > > > > >> > > > > to be used purely from an operational perspective. > > > > >> > > > > > > > > >> > > > > > > > > >> > > > > Does this mean that any such properties are visible to > > > > >> connectors, or > > > > >> > > > will > > > > >> > > > > they be hidden to connectors? Currently no connectors have > > > > access > > > > >> to > > > > >> > > such > > > > >> > > > > client properties, and users are unlike to just put them > > into > > > a > > > > >> > > connector > > > > >> > > > > configuration unnecessarily. A connector implementation > > could > > > > have > > > > >> > > > defined > > > > >> > > > > such properties as normal connector-specific properties, > in > > > > which > > > > >> > case > > > > >> > > > they > > > > >> > > > > are required, but is that likely given the log prefixes? > One > > > > >> concern > > > > >> > > > that I > > > > >> > > > > have is that this might allow connector implementations > > start > > > > >> > > attempting > > > > >> > > > to > > > > >> > > > > circumvent the Connect API if these properties are > included. > > > > >> > > > > > > > > >> > > > > Second, does the None policy allow but ignore these > > additional > > > > >> > > properties > > > > >> > > > > (e.g., "validate(...)" is simply a no-op)? Or does the > None > > > > policy > > > > >> > fail > > > > >> > > > if > > > > >> > > > > any client overrides are specified? The former seems more > in > > > > line > > > > >> > with > > > > >> > > > the > > > > >> > > > > current behavior, whereas the "disallows" policy seems > > useful > > > > but > > > > >> not > > > > >> > > > > exactly backward compatible. Should we also offer a > > "Disallow" > > > > >> > policy? > > > > >> > > In > > > > >> > > > > fact, should the policies be named "Ignore" (default), > > > > "Disallow", > > > > >> > > > > "Prinicipal", and "All"? > > > > >> > > > > > > > > >> > > > > Otherwise, I like the idea of this. There have been > several > > > > >> requests > > > > >> > > over > > > > >> > > > > the past year or two for adding subsets of this > > functionality. > > > > >> Might > > > > >> > be > > > > >> > > > > good to find and list all of the related KAFKA issues. > > > > >> > > > > > > > > >> > > > > Randall > > > > >> > > > > > > > > >> > > > > On Fri, Apr 26, 2019 at 4:04 PM Chris Egerton < > > > > >> chr...@confluent.io> > > > > >> > > > wrote: > > > > >> > > > > > > > > >> > > > > > 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 > > > > >> > > > > > > > > > > >> > > > > > >> > > > > > > > > > > >> > > > > >> > > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > >> > > > > > > > > > > > > > >> > > > > > > > > > > > > >> > > > > > > > > > > > >> > > > > > > > > > > >> > > > > > > > > > >> > > > > > > > > >> > > > > > > > >> > > > > > > >> > > > > > >> > > > > > > > > > > > > > > >
