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 >> > > > >> > > > > > > > > > > >> > >> > > > >> > > > > > > > > > > >> >> > > > >> > > > > > > > > > > > >> > > > >> > > > > > > > > > > >> > > > >> > > > > > > > > > >> > > > >> > > > > > > > > >> > > > >> > > > > > > > >> > > > >> > > > > > > >> > > > >> > > > > > >> > > > >> > > > > >> > > > >> > > > >> > > > >> > > >> > > > >> > >> > > > >> >> > > > > >> > > > >> > > >> > >> >
