Re: [DISCUSS] KIP-215: Add topic regex support for Connect sinks
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-Postavawrote: > 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 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 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 < > > e...@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 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 > 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
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 Klukaswrote: > 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 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 < > e...@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 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 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
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 Klukaswrote: > 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 > 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 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 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
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-Postavawrote: > 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 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 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
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 Yuwrote: > 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 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
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 Klukaswrote: > Looking for feedback on > > https://cwiki.apache.org/confluence/display/KAFKA/KIP- > 215%3A+Add+topic+regex+support+for+Connect+sinks >
[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