Re: [DISCUSS] KAFKA-4930 & KAFKA 4938 - Treatment of name parameter in create connector requests
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 wrote:
> This sounds great. I'll try to review later today :)
>
> On Thu, Jul 6, 2017 at 12:35 AM Sönke Liebau
> 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 >
> > 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
> 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
> > >> 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",
> > >> > "confi
Re: [DISCUSS] KAFKA-4930 & KAFKA 4938 - Treatment of name parameter in create connector requests
This sounds great. I'll try to review later today :)
On Thu, Jul 6, 2017 at 12:35 AM Sönke Liebau
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
> 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 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
> >> 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
Re: [DISCUSS] KAFKA-4930 & KAFKA 4938 - Treatment of name parameter in create connector requests
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
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 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
>> 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
Re: [DISCUSS] KAFKA-4930 & KAFKA 4938 - Treatment of name parameter in create connector requests
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 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
> 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
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
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
>
