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.lie...@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 <g...@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
>> <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
>> >
>>
>
>
>
> --
> 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
