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/dev@kafka.apache.org/msg97124.html>. Thanks, Magesh On Tue, May 7, 2019 at 9:35 AM Magesh Nandakumar <mage...@confluent.io> 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 <mage...@confluent.io> > 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 <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 >>> > > > >> > > > > > > > > > > >> > >>> > > > >> > > > > > > > > > > >> >>> > > > >> > > > > > > > > > > > >>> > > > >> > > > > > > > > > > >>> > > > >> > > > > > > > > > >>> > > > >> > > > > > > > > >>> > > > >> > > > > > > > >>> > > > >> > > > > > > >>> > > > >> > > > > > >>> > > > >> > > > > >>> > > > >> > > > >>> > > > >> > > >>> > > > >> > >>> > > > >> >>> > > > > >>> > > > >>> > > >>> > >>> >>
