Mailing-List: contact dev-help@kafka.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@kafka.apache.org
MIME-Version: 1.0
References: <CAMTc-3GYog=OZ2fbJqf7odgNieqHUH8bG9vQ2RgRjE3CeGe-+Q@mail.gmail.com>
In-Reply-To: <CAMTc-3GYog=OZ2fbJqf7odgNieqHUH8bG9vQ2RgRjE3CeGe-+Q@mail.gmail.com>
From: Gwen Shapira <gwen@confluent.io>
Date: Wed, 05 Jul 2017 03:41:59 +0000
Message-ID: <CAA9QSJLPy6TQLEkDu9dRLt8JTjXh09ZD_DXxM+LPv6mVfMGzLQ@mail.gmail.com>
Subject: Re: [DISCUSS] KAFKA-4930 & KAFKA 4938 - Treatment of name parameter
 in create connector requests
To: Konstantine Karantasis <konstantine@confluent.io>, dev@kafka.apache.org
Content-Type: multipart/alternative; boundary="94eb2c0a814cf204ea055389c835"
archived-at: Wed, 05 Jul 2017 03:42:21 -0000

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

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=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 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 t=
he
> Exception are removed) functionality was to create a connector named "nul=
l"
> 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 connect=
or
> 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 provid=
ed
> 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 m=
ay
> be code out there that references the connector name from the config obje=
ct
> and would thus grab the "wrong" one.
>
> Again, this may be intentional, so I am mostly looking for comments on ho=
w
> 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=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
>

--94eb2c0a814cf204ea055389c835--