kafka-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Boyang Chen <bche...@outlook.com>
Subject Re: [DISCUSS] KIP-276: Add StreamsConfig prefix for different consumers
Date Thu, 05 Apr 2018 05:56:17 GMT
Thanks Guozhang! I already updated the pull request and KIP to deprecate getConsumerConfigs()
function. Do you think we could move to a voting stage now?


________________________________
From: Guozhang Wang <wangguoz@gmail.com>
Sent: Thursday, April 5, 2018 9:52 AM
To: dev@kafka.apache.org
Subject: Re: [DISCUSS] KIP-276: Add StreamsConfig prefix for different consumers

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 <matthias@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 <bchen11@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 <bchen11@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 <matthias@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 <yuzhihong@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 <
> matthias@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 <yuzhihong@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 <bchen11@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

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message