I actually like a separate config for whether to pass or filter client override properties to the connector. I generally dislike adding more properties, but in this case it keeps the two orthogonal behaviors independent and reduces the need to implement policies that cover all permutations.
However, I still find it strange to have a "non-policy" be the default. So either of these modifications to the current KIP would be fine with me: 1) Add a `useOverride()` default method that returns true, but which the None policy could override and return false; and keep the `validate(...)` method as it is. 2) Change the `validate(Map<...>) method to support a filtering pattern, such as `Map<...> filterOverrides(Map<...> connectorClientOverrides)` The point is that the default is the name of a built-in policy. Also, one minor suggestion is to use the term "override" in the config property (e.g., `connector.client.override.policy`) since that term is used prevalently and matches the `producer.override`, `consumer.override`, and `admin.override` prefixes. Thanks for working through this, Magesh. Randall On Mon, May 6, 2019 at 1:11 PM 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 > >> > > > > > > > > > > >> > > >> > > > > > > > > > > >> > >> > > > > > > > > > > > > >> > > > > > > > > > > > >> > > > > > > > > > > >> > > > > > > > > > >> > > > > > > > > >> > > > > > > > >> > > > > > > >> > > > > > >> > > > > >> > > > >> > > >> > > >
