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 > >> > > > >> > > >> > > > > >
