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 <e...@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 <jklu...@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 <jklu...@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 < > > 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 <yuzhih...@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 <j...@klukas.net> > wrote: > > >> > > > >> > > Looking for feedback on > > >> > > > > >> > > https://cwiki.apache.org/confluence/display/KAFKA/KIP- > > >> > > 215%3A+Add+topic+regex+support+for+Connect+sinks > > >> > > > > >> > > > >> > > > > > > > > >
