Could someone have a look at the PR for KAFKA-4930 if they get the chance (not necessarily you Gwen, just bumping in general)? I've updated it according to the latest comments a little while ago and would like to get this done, before I forget what I did in case more changes are necessary :)
Thanks! Jira: https://issues.apache.org/jira/browse/KAFKA-4930 PR: https://github.com/apache/kafka/pull/2755 On Thu, Jul 6, 2017 at 8:19 PM, Gwen Shapira <g...@confluent.io> wrote: > This sounds great. I'll try to review later today :) > > On Thu, Jul 6, 2017 at 12:35 AM Sönke Liebau > <soenke.lie...@opencore.com.invalid> wrote: > > > I've updated the pull request to behave as follows: > > - reject create requests that contain no "name" element with a > > BadRequestException > > - reject name that are empty or contain illegal characters with a > > ConfigException > > - leave current logic around when to copy the name from the create > request > > to the config element intact > > - added unit tests for the validator to check that illegal characters > are > > correctly identified > > > > The list of illegal characters is the result of some quick testing I did, > > all of the characters in the list currently cause issues when used in a > > connector name (similar to KAFKA-4827), so this should not break anything > > that anybody relies on. > > I think we may want to start a larger discussion around connector names, > > allowed characters, max length, .. to come up with an airtight set of > > rules that we can then enforce, I am sure this is currently not perfect > as > > is. > > > > Best regards, > > Sönke > > > > On Wed, Jul 5, 2017 at 9:31 AM, Sönke Liebau <soenke.lie...@opencore.com > > > > wrote: > > > > > Hi, > > > > > > regarding "breaking existing functionality" .. yes...that was me > getting > > > confused about intended and existing functionality :) > > > You are right, this won't break anything that is currently working. > > > > > > I'll leave placement of "name" parameter as is and open a new issue to > > > clarify this later on. > > > > > > Kind regards, > > > Sönke > > > > > > On Wed, Jul 5, 2017 at 5:41 AM, Gwen Shapira <g...@confluent.io> > wrote: > > > > > >> Hey, > > >> > > >> Nice research and summary. > > >> > > >> Regarding the ability to have a "nameless" connector - I'm pretty sure > > we > > >> never intended to allow that. > > >> I'm confused about breaking something that currently works though - > > since > > >> we get NPE, how will giving more intentional exceptions break > anything? > > >> > > >> Regarding the placing of the name - inside or outside the config. It > > looks > > >> messy and I'm as confused as you are. I think Konstantine had some > ideas > > >> how this should be resolved. I hope he responds, but I think that for > > your > > >> PR, just accept current mess as given... > > >> > > >> Gwen > > >> > > >> On Tue, Jul 4, 2017 at 3:28 AM Sönke Liebau > > >> <soenke.lie...@opencore.com.invalid> wrote: > > >> > > >> > While working on KAFKA-4930 and KAFKA-4938 I came across some sort > of > > >> > fundamental questions about the rest api for creating connectors in > > >> Kafka > > >> > Connect that I'd like to put up for discussion. > > >> > > > >> > Currently requests that do not contain a "name" element on the top > > level > > >> > are not accepted by the API, but that is due to a > NullPointerException > > >> [1] > > >> > so not entirely intentional. Previous (and current if the lines > > causing > > >> the > > >> > Exception are removed) functionality was to create a connector named > > >> "null" > > >> > if that parameter was missing. I am not sure if this is a good > thing, > > as > > >> > for example that connector will be overwritten every time a new > > request > > >> > without a name is sent, as opposed to the expected warning that a > > >> connector > > >> > of that name already exists. > > >> > > > >> > I would propose to reject api calls without a name provided on the > top > > >> > level, but this might break requests that currently work, so should > > >> > probably be mentioned in the release notes. > > >> > > > >> > ---- > > >> > > > >> > Additionally, the "name" parameter is also copied into the "config" > > >> > sub-element of the connector request - unless a name parameter was > > >> provided > > >> > there in the original request[2]. > > >> > > > >> > So this: > > >> > > > >> > { > > >> > "name": "connectorname", > > >> > "config": { > > >> > "connector.class": > > >> > "org.apache.kafka.connect.tools.MockSourceConnector", > > >> > "tasks.max": "1", > > >> > "topics": "test-topic" > > >> > } > > >> > } > > >> > > > >> > would become this: > > >> > { > > >> > "name": "connectorname", > > >> > "config": { > > >> > "name": "connectorname", > > >> > "connector.class": > > >> > "org.apache.kafka.connect.tools.MockSourceConnector", > > >> > "tasks.max": "1", > > >> > "topics": "test-topic" > > >> > } > > >> > } > > >> > > > >> > But a request that contains two different names like this: > > >> > > > >> > { > > >> > "name": "connectorname", > > >> > "config": { > > >> > "name": "differentconnectorname", > > >> > "connector.class": > > >> > "org.apache.kafka.connect.tools.MockSourceConnector", > > >> > "tasks.max": "1", > > >> > "topics": "test-topic" > > >> > } > > >> > } > > >> > > > >> > would be allowed as is. > > >> > > > >> > This might be intentional behavior in order to enable Connectors to > > >> have a > > >> > "name" parameter of their own - though I couldn't find any that do, > > but > > >> I > > >> > think this has the potential for misunderstandings, especially as > > there > > >> may > > >> > be code out there that references the connector name from the config > > >> object > > >> > and would thus grab the "wrong" one. > > >> > > > >> > Again, this may be intentional, so I am mostly looking for comments > on > > >> how > > >> > to proceed here. > > >> > > > >> > My first instinct is to make the top-level "name" parameter > mandatory > > as > > >> > suggested above and then add a check to reject requests that > contain a > > >> > different "name" field in the config element. > > >> > > > >> > Any comments or thoughts welcome. > > >> > > > >> > TL/DR: > > >> > Two questions up for discussion: > > >> > 1. Should we reject api calls to create a connector that do not > > contain > > >> a > > >> > "name" element on the top level? > > >> > 2. Is there a use case where it makes sense to have different "name" > > >> > elements in the connector config and as the connector name? > > >> > > > >> > Kind regards, > > >> > Sönke > > >> > > > >> > [1] https://github.com/apache/kafka/blob/trunk/connect/ > > >> > runtime/src/main/java/org/apache/kafka/connect/runtime/ > rest/resources/ > > >> > ConnectorsResource.java#L91 > > >> > > > >> > [2] https://github.com/apache/kafka/blob/trunk/connect/ > > >> > runtime/src/main/java/org/apache/kafka/connect/runtime/ > rest/resources/ > > >> > ConnectorsResource.java#L96 > > >> > > > >> > > > > > > > > > > > > -- > > > Sönke Liebau > > > Partner > > > Tel. +49 179 7940878 <+49%20179%207940878> <+49%20179%207940878> > > > OpenCore GmbH & Co. KG - Thomas-Mann-Straße 8 - 22880 Wedel - Germany > > > > > > > > > > > -- > > Sönke Liebau > > Partner > > Tel. +49 179 7940878 <+49%20179%207940878> > > OpenCore GmbH & Co. KG - Thomas-Mann-Straße 8 - 22880 Wedel - Germany > > > -- Sönke Liebau Partner Tel. +49 179 7940878 OpenCore GmbH & Co. KG - Thomas-Mann-Straße 8 - 22880 Wedel - Germany
