Thanks for updating the KIP. One more comment. Even if we don't expect users to call `StreamsConfig#getConsumerConfigs()` it is still public API. Thus, we cannot just rename the method.
I think we have two options: either we keep the current name or we deprecate the method and introduce `getMainConsumerConfigs()` in parallel. The deprecated method would just call the new method. I am not sure if we gain a lot renaming the method, thus I have a slight preference in just keeping the method name as is. (But I am also ok to rename it, if people prefer this) -Matthias On 4/3/18 1:59 PM, Bill Bejeck wrote: > Boyang, > > Thanks for making changes to the KIP, I'm +1 on the updated version. > > -Bill > > On Tue, Apr 3, 2018 at 1:14 AM, Boyang Chen <bche...@outlook.com> wrote: > >> 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 >>>>>>>> >>>>>>> >>>>>> >>>>>> >>>>>> >>>>> >>>>> >>>> >>> >>> >>> >> >> >
signature.asc
Description: OpenPGP digital signature
