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
>
