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