kafka-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Bill Bejeck <bbej...@gmail.com>
Subject Re: [DISCUSS] KIP-276: Add StreamsConfig prefix for different consumers
Date Tue, 03 Apr 2018 20:59:19 GMT
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
> >>>>>>
> >>>>>
> >>>>
> >>>>
> >>>>
> >>>
> >>>
> >>
> >
> >
> >
>
>

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