You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Jeff Klukas <je...@klukas.net> on 2017/10/27 01:27:26 UTC
[DISCUSS] KIP-215: Add topic regex support for Connect sinks
Looking for feedback on
https://cwiki.apache.org/confluence/display/KAFKA/KIP-215%3A+Add+topic+regex+support+for+Connect+sinks
Re: [DISCUSS] KIP-215: Add topic regex support for Connect sinks
Posted by Jeff Klukas <jk...@simple.com>.
I responded to Ewen's suggestions in the PR and went back to using
ConfigException.
If I don't hear any other concerns today, I'll start a [VOTE] thread for
the KIP.
On Mon, Oct 30, 2017 at 9:29 PM, Ewen Cheslack-Postava <ew...@confluent.io>
wrote:
> I took a quick pass at the PR, looks good so far. ConfigException would
> still be fine in the case you're highlighting as it's inside the framework
> anyway and we'd expect a ConfigException from configure() if connectors try
> to use their ConfigDef to parse an invalid config. But here I don't feel
> strongly about which to use since the error message is clear anyway and
> will just end up in logs / the REST API for the user to sort out.
>
> -Ewen
>
> On Fri, Oct 27, 2017 at 6:39 PM, Jeff Klukas <jk...@simple.com> wrote:
>
> > I've updated the KIP to use the topics.regex name and opened a WIP PR
> with
> > an implementation that shows some additional complexity in how the
> > configuration option gets passed through, affecting various public
> function
> > signatures.
> >
> > I would appreciate any eyes on that for feedback on whether more design
> > discussion needs to happen in the KIP.
> >
> > https://github.com/apache/kafka/pull/4151
> >
> > On Fri, Oct 27, 2017 at 7:50 AM, Jeff Klukas <jk...@simple.com> wrote:
> >
> > > I added a note in the KIP about ConfigException being thrown. I also
> > > changed the proposed default for the new config to empty string rather
> > than
> > > null.
> > >
> > > Absent a clear definition of what "common" regex syntax is, it seems an
> > > undue burden to ask the user to guess at what Pattern features are
> safe.
> > If
> > > we do end up implementing a different regex style, I think it will be
> > > necessary to still support the Java Pattern style long-term as an
> option.
> > > If we want to use a different regex style as default down the road, we
> > > could require "power users" of Java Pattern to enable an additional
> > config
> > > option to maintain compatibility.
> > >
> > > One additional change I might make to the KIP is that 'topics.regex'
> > might
> > > be a better choice for config name than 'topics.pattern'. That would be
> > in
> > > keeping with RegexRouter that has a 'regex' configuration option rather
> > > than 'pattern'.
> > >
> > > On Thu, Oct 26, 2017 at 11:00 PM, Ewen Cheslack-Postava <
> > ewen@confluent.io
> > > > wrote:
> > >
> > >> It's fine to be more detailed, but ConfigException is already implied
> > for
> > >> all other config issues as well.
> > >>
> > >> Default could be either null or just empty string. re: alternatives,
> if
> > >> you
> > >> wanted to be slightly more detailed (though still a bit vague) re:
> > >> supported syntax, you could just say that while Pattern is used, we
> only
> > >> guarantee support for common regular expression syntax. Not sure if
> > >> there's
> > >> a good way of defining what "common" syntax is.
> > >>
> > >> Otherwise LGTM, and thanks for helping fill in a longstanding gap!
> > >>
> > >> -Ewen
> > >>
> > >> On Thu, Oct 26, 2017 at 7:56 PM, Ted Yu <yu...@gmail.com> wrote:
> > >>
> > >> > bq. Users may specify only one of 'topics' or 'topics.pattern'.
> > >> >
> > >> > Can you fill in which exception would be thrown if both of them are
> > >> > specified
> > >> > ?
> > >> >
> > >> > Cheers
> > >> >
> > >> > On Thu, Oct 26, 2017 at 6:27 PM, Jeff Klukas <je...@klukas.net>
> wrote:
> > >> >
> > >> > > Looking for feedback on
> > >> > >
> > >> > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> > >> > > 215%3A+Add+topic+regex+support+for+Connect+sinks
> > >> > >
> > >> >
> > >>
> > >
> > >
> >
>
Re: [DISCUSS] KIP-215: Add topic regex support for Connect sinks
Posted by Ewen Cheslack-Postava <ew...@confluent.io>.
I took a quick pass at the PR, looks good so far. ConfigException would
still be fine in the case you're highlighting as it's inside the framework
anyway and we'd expect a ConfigException from configure() if connectors try
to use their ConfigDef to parse an invalid config. But here I don't feel
strongly about which to use since the error message is clear anyway and
will just end up in logs / the REST API for the user to sort out.
-Ewen
On Fri, Oct 27, 2017 at 6:39 PM, Jeff Klukas <jk...@simple.com> wrote:
> I've updated the KIP to use the topics.regex name and opened a WIP PR with
> an implementation that shows some additional complexity in how the
> configuration option gets passed through, affecting various public function
> signatures.
>
> I would appreciate any eyes on that for feedback on whether more design
> discussion needs to happen in the KIP.
>
> https://github.com/apache/kafka/pull/4151
>
> On Fri, Oct 27, 2017 at 7:50 AM, Jeff Klukas <jk...@simple.com> wrote:
>
> > I added a note in the KIP about ConfigException being thrown. I also
> > changed the proposed default for the new config to empty string rather
> than
> > null.
> >
> > Absent a clear definition of what "common" regex syntax is, it seems an
> > undue burden to ask the user to guess at what Pattern features are safe.
> If
> > we do end up implementing a different regex style, I think it will be
> > necessary to still support the Java Pattern style long-term as an option.
> > If we want to use a different regex style as default down the road, we
> > could require "power users" of Java Pattern to enable an additional
> config
> > option to maintain compatibility.
> >
> > One additional change I might make to the KIP is that 'topics.regex'
> might
> > be a better choice for config name than 'topics.pattern'. That would be
> in
> > keeping with RegexRouter that has a 'regex' configuration option rather
> > than 'pattern'.
> >
> > On Thu, Oct 26, 2017 at 11:00 PM, Ewen Cheslack-Postava <
> ewen@confluent.io
> > > wrote:
> >
> >> It's fine to be more detailed, but ConfigException is already implied
> for
> >> all other config issues as well.
> >>
> >> Default could be either null or just empty string. re: alternatives, if
> >> you
> >> wanted to be slightly more detailed (though still a bit vague) re:
> >> supported syntax, you could just say that while Pattern is used, we only
> >> guarantee support for common regular expression syntax. Not sure if
> >> there's
> >> a good way of defining what "common" syntax is.
> >>
> >> Otherwise LGTM, and thanks for helping fill in a longstanding gap!
> >>
> >> -Ewen
> >>
> >> On Thu, Oct 26, 2017 at 7:56 PM, Ted Yu <yu...@gmail.com> wrote:
> >>
> >> > bq. Users may specify only one of 'topics' or 'topics.pattern'.
> >> >
> >> > Can you fill in which exception would be thrown if both of them are
> >> > specified
> >> > ?
> >> >
> >> > Cheers
> >> >
> >> > On Thu, Oct 26, 2017 at 6:27 PM, Jeff Klukas <je...@klukas.net> wrote:
> >> >
> >> > > Looking for feedback on
> >> > >
> >> > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> >> > > 215%3A+Add+topic+regex+support+for+Connect+sinks
> >> > >
> >> >
> >>
> >
> >
>
Re: [DISCUSS] KIP-215: Add topic regex support for Connect sinks
Posted by Jeff Klukas <jk...@simple.com>.
I've updated the KIP to use the topics.regex name and opened a WIP PR with
an implementation that shows some additional complexity in how the
configuration option gets passed through, affecting various public function
signatures.
I would appreciate any eyes on that for feedback on whether more design
discussion needs to happen in the KIP.
https://github.com/apache/kafka/pull/4151
On Fri, Oct 27, 2017 at 7:50 AM, Jeff Klukas <jk...@simple.com> wrote:
> I added a note in the KIP about ConfigException being thrown. I also
> changed the proposed default for the new config to empty string rather than
> null.
>
> Absent a clear definition of what "common" regex syntax is, it seems an
> undue burden to ask the user to guess at what Pattern features are safe. If
> we do end up implementing a different regex style, I think it will be
> necessary to still support the Java Pattern style long-term as an option.
> If we want to use a different regex style as default down the road, we
> could require "power users" of Java Pattern to enable an additional config
> option to maintain compatibility.
>
> One additional change I might make to the KIP is that 'topics.regex' might
> be a better choice for config name than 'topics.pattern'. That would be in
> keeping with RegexRouter that has a 'regex' configuration option rather
> than 'pattern'.
>
> On Thu, Oct 26, 2017 at 11:00 PM, Ewen Cheslack-Postava <ewen@confluent.io
> > wrote:
>
>> It's fine to be more detailed, but ConfigException is already implied for
>> all other config issues as well.
>>
>> Default could be either null or just empty string. re: alternatives, if
>> you
>> wanted to be slightly more detailed (though still a bit vague) re:
>> supported syntax, you could just say that while Pattern is used, we only
>> guarantee support for common regular expression syntax. Not sure if
>> there's
>> a good way of defining what "common" syntax is.
>>
>> Otherwise LGTM, and thanks for helping fill in a longstanding gap!
>>
>> -Ewen
>>
>> On Thu, Oct 26, 2017 at 7:56 PM, Ted Yu <yu...@gmail.com> wrote:
>>
>> > bq. Users may specify only one of 'topics' or 'topics.pattern'.
>> >
>> > Can you fill in which exception would be thrown if both of them are
>> > specified
>> > ?
>> >
>> > Cheers
>> >
>> > On Thu, Oct 26, 2017 at 6:27 PM, Jeff Klukas <je...@klukas.net> wrote:
>> >
>> > > Looking for feedback on
>> > >
>> > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-
>> > > 215%3A+Add+topic+regex+support+for+Connect+sinks
>> > >
>> >
>>
>
>
Re: [DISCUSS] KIP-215: Add topic regex support for Connect sinks
Posted by Jeff Klukas <jk...@simple.com>.
I added a note in the KIP about ConfigException being thrown. I also
changed the proposed default for the new config to empty string rather than
null.
Absent a clear definition of what "common" regex syntax is, it seems an
undue burden to ask the user to guess at what Pattern features are safe. If
we do end up implementing a different regex style, I think it will be
necessary to still support the Java Pattern style long-term as an option.
If we want to use a different regex style as default down the road, we
could require "power users" of Java Pattern to enable an additional config
option to maintain compatibility.
One additional change I might make to the KIP is that 'topics.regex' might
be a better choice for config name than 'topics.pattern'. That would be in
keeping with RegexRouter that has a 'regex' configuration option rather
than 'pattern'.
On Thu, Oct 26, 2017 at 11:00 PM, Ewen Cheslack-Postava <ew...@confluent.io>
wrote:
> It's fine to be more detailed, but ConfigException is already implied for
> all other config issues as well.
>
> Default could be either null or just empty string. re: alternatives, if you
> wanted to be slightly more detailed (though still a bit vague) re:
> supported syntax, you could just say that while Pattern is used, we only
> guarantee support for common regular expression syntax. Not sure if there's
> a good way of defining what "common" syntax is.
>
> Otherwise LGTM, and thanks for helping fill in a longstanding gap!
>
> -Ewen
>
> On Thu, Oct 26, 2017 at 7:56 PM, Ted Yu <yu...@gmail.com> wrote:
>
> > bq. Users may specify only one of 'topics' or 'topics.pattern'.
> >
> > Can you fill in which exception would be thrown if both of them are
> > specified
> > ?
> >
> > Cheers
> >
> > On Thu, Oct 26, 2017 at 6:27 PM, Jeff Klukas <je...@klukas.net> wrote:
> >
> > > Looking for feedback on
> > >
> > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> > > 215%3A+Add+topic+regex+support+for+Connect+sinks
> > >
> >
>
Re: [DISCUSS] KIP-215: Add topic regex support for Connect sinks
Posted by Ewen Cheslack-Postava <ew...@confluent.io>.
It's fine to be more detailed, but ConfigException is already implied for
all other config issues as well.
Default could be either null or just empty string. re: alternatives, if you
wanted to be slightly more detailed (though still a bit vague) re:
supported syntax, you could just say that while Pattern is used, we only
guarantee support for common regular expression syntax. Not sure if there's
a good way of defining what "common" syntax is.
Otherwise LGTM, and thanks for helping fill in a longstanding gap!
-Ewen
On Thu, Oct 26, 2017 at 7:56 PM, Ted Yu <yu...@gmail.com> wrote:
> bq. Users may specify only one of 'topics' or 'topics.pattern'.
>
> Can you fill in which exception would be thrown if both of them are
> specified
> ?
>
> Cheers
>
> On Thu, Oct 26, 2017 at 6:27 PM, Jeff Klukas <je...@klukas.net> wrote:
>
> > Looking for feedback on
> >
> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> > 215%3A+Add+topic+regex+support+for+Connect+sinks
> >
>
Re: [DISCUSS] KIP-215: Add topic regex support for Connect sinks
Posted by Ted Yu <yu...@gmail.com>.
bq. Users may specify only one of 'topics' or 'topics.pattern'.
Can you fill in which exception would be thrown if both of them are specified
?
Cheers
On Thu, Oct 26, 2017 at 6:27 PM, Jeff Klukas <je...@klukas.net> wrote:
> Looking for feedback on
>
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> 215%3A+Add+topic+regex+support+for+Connect+sinks
>