Mailing-List: contact dev-help@kafka.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@kafka.apache.org
MIME-Version: 1.0
In-Reply-To: <CAA9QSJ+req+3mWFA6n-g8-3S6kDmf6wZjqKGD=N5zogAsgb+DA@mail.gmail.com>
References: <CAMTc-3GYog=OZ2fbJqf7odgNieqHUH8bG9vQ2RgRjE3CeGe-+Q@mail.gmail.com>
 <CAA9QSJLPy6TQLEkDu9dRLt8JTjXh09ZD_DXxM+LPv6mVfMGzLQ@mail.gmail.com>
 <CAMTc-3EoYvktp=nd4ZwH+_obEhU87HOzvFzj2AZMSa3iU2bUpw@mail.gmail.com>
 <CAMTc-3GtnYC2HsyTkWE-foHwDqsa=ciRrYyfmPB7iC_3qqk+pA@mail.gmail.com> <CAA9QSJ+req+3mWFA6n-g8-3S6kDmf6wZjqKGD=N5zogAsgb+DA@mail.gmail.com>
From: =?UTF-8?Q?S=C3=B6nke_Liebau?= <soenke.liebau@opencore.com.INVALID>
Date: Wed, 9 Aug 2017 16:43:08 +0200
Message-ID: <CAMTc-3HCF8sDGT=pjBgQ0wsFVEbL7X-wEdPbh1-FLQx4KksrDg@mail.gmail.com>
Subject: Re: [DISCUSS] KAFKA-4930 & KAFKA 4938 - Treatment of name parameter
 in create connector requests
To: dev@kafka.apache.org
Content-Type: multipart/alternative; boundary="94eb2c13f9909419640556531b66"
archived-at: Wed, 09 Aug 2017 14:43:57 -0000

--94eb2c13f9909419640556531b66
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

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 <gwen@confluent.io> wrote:

> This sounds great. I'll try to review later today :)
>
> On Thu, Jul 6, 2017 at 12:35 AM S=C3=B6nke Liebau
> <soenke.liebau@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 di=
d,
> > 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 anythi=
ng
> > 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=C3=B6nke
> >
> > On Wed, Jul 5, 2017 at 9:31 AM, S=C3=B6nke Liebau <soenke.liebau@openco=
re.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 t=
o
> > > clarify this later on.
> > >
> > > Kind regards,
> > > S=C3=B6nke
> > >
> > > On Wed, Jul 5, 2017 at 5:41 AM, Gwen Shapira <gwen@confluent.io>
> wrote:
> > >
> > >> Hey,
> > >>
> > >> Nice research and summary.
> > >>
> > >> Regarding the ability to have a "nameless" connector - I'm pretty su=
re
> > 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 fo=
r
> > your
> > >> PR, just accept current mess as given...
> > >>
> > >> Gwen
> > >>
> > >> On Tue, Jul 4, 2017 at 3:28 AM S=C3=B6nke Liebau
> > >> <soenke.liebau@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 i=
n
> > >> 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 nam=
ed
> > >> "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 shoul=
d
> > >> > 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 t=
o
> > >> 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 conf=
ig
> > >> object
> > >> > and would thus grab the "wrong" one.
> > >> >
> > >> > Again, this may be intentional, so I am mostly looking for comment=
s
> 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 "nam=
e"
> > >> > elements in the connector config and as the connector name?
> > >> >
> > >> > Kind regards,
> > >> > S=C3=B6nke
> > >> >
> > >> > [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=C3=B6nke Liebau
> > > Partner
> > > Tel. +49 179 7940878 <+49%20179%207940878> <+49%20179%207940878>
> > > OpenCore GmbH & Co. KG - Thomas-Mann-Stra=C3=9Fe 8 - 22880 Wedel - Ge=
rmany
> > >
> >
> >
> >
> > --
> > S=C3=B6nke Liebau
> > Partner
> > Tel. +49 179 7940878 <+49%20179%207940878>
> > OpenCore GmbH & Co. KG - Thomas-Mann-Stra=C3=9Fe 8 - 22880 Wedel - Germ=
any
> >
>


--=20
S=C3=B6nke Liebau
Partner
Tel. +49 179 7940878
OpenCore GmbH & Co. KG - Thomas-Mann-Stra=C3=9Fe 8 - 22880 Wedel - Germany

--94eb2c13f9909419640556531b66--