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 >>> >> > > >
signature.asc
Description: OpenPGP digital signature
