Re: [DISCUSS] KAFKA-4930 & KAFKA 4938 - Treatment of name parameter in create connector requests

[Sönke Liebau <so...@opencore.com.INVALID>]

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 <gw...@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
> <so...@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.liebau@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 <gw...@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
> > >> <so...@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