Hey friends,
both KIP<https://cwiki.apache.org/confluence/display/KAFKA/KIP-276+Add+StreamsConfig+prefix+for+different+consumers> and pull request<https://github.com/apache/kafka/pull/4805> are updated. Feel free to take another look. Thanks for your valuable feedback! Best, Boyang ________________________________ From: Boyang Chen <bche...@outlook.com> Sent: Tuesday, April 3, 2018 11:39 AM To: dev@kafka.apache.org Subject: Re: [DISCUSS] KIP-276: Add StreamsConfig prefix for different consumers Thanks Matthias, Ted and Guozhang for the inputs. I shall address them in next round. ________________________________ From: Matthias J. Sax <matth...@confluent.io> Sent: Tuesday, April 3, 2018 4:43 AM To: dev@kafka.apache.org Subject: Re: [DISCUSS] KIP-276: Add StreamsConfig prefix for different consumers Yes, your examples make sense to me. That was the idea behind the proposal. -Matthias On 4/2/18 11:25 AM, Guozhang Wang wrote: > @Matthias > > That's a good question: I think adding another config for the main consumer > makes good tradeoffs between compatibility and new user convenience. Just > to clarify, from user's pov on upgrade: > > 1) I'm already overriding some consumer configs, and now I want to override > these values differently for restore consumers, I'd add one new line for > the restore consumer prefix. > > 2) I'm already overriding some consumer configs, and now I want to NOT > overriding them for restore consumers, I'd change my override from > `consumer.X` to `main.consumer.X`. > > 3) I'm new and have not any consumer overrides, and now if I want to > override some, I'd use `main.consumer`, `restore.consumer` for specific > consumer types, and ONLY consider `consumer` for the ones that I want to > apply universally. > > 4) I'm already overriding some consumer configs and I'm happy with what I > get, I do not change anything. > > > Guozhang > > On Mon, Apr 2, 2018 at 11:10 AM, Ted Yu <yuzhih...@gmail.com> wrote: > >> bq. to introduce one more prefix `main.consumer.` >> >> Makes sense. >> >> On Mon, Apr 2, 2018 at 11:06 AM, Matthias J. Sax <matth...@confluent.io> >> wrote: >> >>> Boyang, >>> >>> thanks a lot for the KIP. >>> >>> Couple of questions: >>> >>>> (MODIFIED) public Map<String, Object> getRestoreConsumerConfigs(final >>> String clientId); >>> >>> You mean that the implementation/semantics of this method changes, but >>> not the API itself? Might be good to add this as "comment style" instead >>> of "(MODIFIED)" prefix. >>> >>>> By rewriting the getRestoreConsumerConfigs() function and adding the >>> getGlobalConsumerConfigs() function, if one user uses >>> restoreConsumerPrefix() or globalConsumerPrefix() when adding new >>> configurations, the configs shall overwrite base consumer config. If not >>> specified, restore consumer and global consumer shall share the same >> config >>> with base consumer. >>> >>> While this does make sense for backward compatibility, I am wonder if it >>> makes the config "inheritance logic" (ie, hierarchy) too complex? We >>> basically introduce a second level of overwrites. It might be simpler to >>> not introduce this hierarchy with the cost to break backward >> compatibility. >>> >>> For example, config `request.timeout.ms`: >>> >>> User sets `request.timeout.ms=<user-value>` >>> To change it for the main consumer, user also sets >>> `consumer.request.timeout.ms=<consumer-value>` >>> >>> If user only wants to change the config for main consumer, but not for >>> global or restore consumer, user needs to add two more configs: >>> >>> `restore.consumer.request.timeout.ms=<user-value>` >>> and >>> `global.consumer.request.timeout.ms=<user-value>` >>> >>> to reset both back to the default config. IMHO, this is not an optimal >>> user experience. Thus, it might be worth to change the semantics for >>> `consumer.` prefix to only apply those configs to the main consumer. >>> >>> >>> Not sure what other think what the better solution is (I am not sure by >>> myself to be honest---just wanted to point it out and discuss the >>> pros/cons for both). >>> >>> >>> Another though would be, to introduce one more prefix `main.consumer.` >>> -- using this, the existing `consumer.` prefix would apply to all >>> consumers (keeping it's current semantics) while we have overwrites for >>> all three consumers -- this allow to directly set `main.consumer` >>> instead of `consumer` avoiding the weird pattern from my example above >>> and preserves backward compatibility. Ie, if we introduce an hierarchy >>> of overwrite, a "full" hierarchy might be better than a "partial" >>> hierarchy. >>> >>> >>> Looking forward to your thoughts. >>> >>> >>> -Matthias >>> >>> >>> On 4/1/18 5:55 PM, Guozhang Wang wrote: >>>> Thanks for the KIP Boyang, the KIP looks good to me. >>>> >>>> For config values, we use underscore for keeping a single word; for >>> config >>>> keys, though, we do not use underscores or dashes. I'd suggest using >> dots >>>> to be consistent with others. >>>> >>>> >>>> Otherwise I'm +1 on the KIP. >>>> >>>> >>>> Guozhang >>>> >>>> >>>> On Sun, Apr 1, 2018 at 10:56 AM, Ted Yu <yuzhih...@gmail.com> wrote: >>>> >>>>> Looks good overall. >>>>> >>>>> public static final String RESTORE_CONSUMER_PREFIX = >>> "restore-consumer."; >>>>> >>>>> For other constants in StreamsConfig, underscore is used instead of >>> dash. >>>>> >>>>> Cheers >>>>> >>>>> On Sun, Apr 1, 2018 at 9:38 AM, Boyang Chen <bche...@outlook.com> >>> wrote: >>>>> >>>>>> Hey friends, >>>>>> >>>>>> >>>>>> I would like to start a discussion thread for KIP 276: >>>>>> >>>>>> https://cwiki.apache.org/confluence/display/KAFKA/KIP- >>>>>> 276+Add+StreamsConfig+prefix+for+different+consumers >>>>>> >>>>>> >>>>>> And pull request is here: >>>>>> >>>>>> https://github.com/apache/kafka/pull/4805 >>>>>> >>>>>> [https://avatars3.githubusercontent.com/u/5845561?s=400&v=4 >> ]<https:// >>>>>> github.com/apache/kafka/pull/4805> >>>>>> >>>>>> KAFKA-6657: Add StreamsConfig prefix for different consumers by >>> abbccdda >>>>> · >>>>>> Pull Request #4805 · apache/kafka<https://github. >>>>>> com/apache/kafka/pull/4805> >>>>>> github.com >>>>>> This pull request is for jira 6657. The KIP proposal is here Added >> unit >>>>>> tests for new getGlobalConsumerConfigs API and make sure existing >>> restore >>>>>> consumer tests are passing. >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> Thanks, >>>>>> >>>>>> Boyang >>>>>> >>>>> >>>> >>>> >>>> >>> >>> >> > > >
