Hi all, I'd like to bring attention to a feature that was added as part of the KIP but never actually documented: the new "admin." prefix and how it's used by workers to compute the configuration for admin clients when using the DLQ for a connector. I've written up a more detailed description of the issue, including a proposed fix, in a Jira that you can find here: https://issues.apache.org/jira/browse/KAFKA-9046. Would be interested to hear people's thoughts on the matter, especially with the 2.4 release coming up soon; if we move quickly, it may be possible to get a fix in (if one is deemed necessary) in time for the release.
Cheers, Chris On Tue, May 7, 2019 at 9:44 AM Magesh Nandakumar <[email protected]> wrote: > Hi All, > > I would like to start a vote on > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-458%3A+Connector+Client+Config+Override+Policy > > The discussion thread can be found here > <https://www.mail-archive.com/[email protected]/msg97124.html>. > > Thanks, > Magesh > > On Tue, May 7, 2019 at 9:35 AM Magesh Nandakumar <[email protected]> > wrote: > > > I have addressed all the outstanding discussion points and I believe we > > have consensus on the design. Thanks, everyone for the feedback. I will > > start a VOTE thread on this KIP. > > > > On Mon, May 6, 2019 at 10:13 PM Magesh Nandakumar <[email protected]> > > wrote: > > > >> 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 <[email protected]> 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 <[email protected] > > > >>> 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 < > >>> > [email protected]> 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 < > >>> [email protected]> > >>> > > 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 < > >>> > [email protected]> > >>> > > > 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 < > [email protected]> > >>> > > 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 < > >>> [email protected]> > >>> > > > >> 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 <[email protected] > > > >>> > wrote: > >>> > > > >> > > >>> > > > >> > > On Tue, Apr 30, 2019 at 4:20 PM Magesh Nandakumar < > >>> > > > >> [email protected]> > >>> > > > >> > > 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 < > >>> > [email protected] > >>> > > > > >>> > > > >> > 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 < > >>> > > > >> [email protected]> > >>> > > > >> > > > 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 < > >>> > > > >> > > > [email protected]> > >>> > > > >> > > > > > 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 < > >>> > > > >> > [email protected] > >>> > > > >> > > > > >>> > > > >> > > > > > 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 < > >>> > > > >> > > > > > [email protected] > >>> > > > >> > > > > > > > > >>> > > > >> > > > > > > > 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 < > >>> > > > >> > > > > [email protected]> > >>> > > > >> > > > > > > > > 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 < > >>> > > > >> > > > > > > > [email protected] > >>> > > > >> > > > > > > > > > > >>> > > > >> > > > > > > > > > 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 < > >>> > > > >> > > > > > > > > [email protected] > >>> > > > >> > > > > > > > > > > > >>> > > > >> > > > > > > > > > > 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 > >>> > < > >>> > > > >> > > > > > > [email protected] > >>> > > > >> > > > > > > > > > >>> > > > >> > > > > > > > > > > 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 < > >>> > > > >> > > > > > > > > > [email protected] > >>> > > > >> > > > > > > > > > > > > >>> > > > >> > > > > > > > > > > >> 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 > >>> > > > >> > > > > > > > > > > >> > > >>> > > > >> > > > > > > > > > > >> > >>> > > > >> > > > > > > > > > > > > >>> > > > >> > > > > > > > > > > > >>> > > > >> > > > > > > > > > > >>> > > > >> > > > > > > > > > >>> > > > >> > > > > > > > > >>> > > > >> > > > > > > > >>> > > > >> > > > > > > >>> > > > >> > > > > > >>> > > > >> > > > > >>> > > > >> > > > >>> > > > >> > > >>> > > > >> > >>> > > > > > >>> > > > > >>> > > > >>> > > >>> > >> >
