You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Sönke Liebau <so...@opencore.com.INVALID> on 2017/07/04 10:28:10 UTC
[DISCUSS] KAFKA-4930 & KAFKA 4938 - Treatment of name parameter in
create connector requests
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
Re: [DISCUSS] KAFKA-4930 & KAFKA 4938 - Treatment of name parameter
in create connector requests
Posted by 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
Re: [DISCUSS] KAFKA-4930 & KAFKA 4938 - Treatment of name parameter
in create connector requests
Posted by Gwen Shapira <gw...@confluent.io>.
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 <so...@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
>
Re: [DISCUSS] KAFKA-4930 & KAFKA 4938 - Treatment of name parameter
in create connector requests
Posted by Sönke Liebau <so...@opencore.com.INVALID>.
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 <so...@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>
> 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
Re: [DISCUSS] KAFKA-4930 & KAFKA 4938 - Treatment of name parameter
in create connector requests
Posted by Sönke Liebau <so...@opencore.com.INVALID>.
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
OpenCore GmbH & Co. KG - Thomas-Mann-Straße 8 - 22880 Wedel - Germany
Re: [DISCUSS] KAFKA-4930 & KAFKA 4938 - Treatment of name parameter
in create connector requests
Posted by Gwen Shapira <gw...@confluent.io>.
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
>