I agree that renaming the method in this case may not worth it. Let's keep the existing function names.
On Wed, Apr 4, 2018 at 6:06 PM, Matthias J. Sax <matth...@confluent.io> wrote: > 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 > >>>>>>>> > >>>>>>> > >>>>>> > >>>>>> > >>>>>> > >>>>> > >>>>> > >>>> > >>> > >>> > >>> > >> > >> > > > > -- -- Guozhang
