This sounds great. I'll try to review later today :)
On Thu, Jul 6, 2017 at 12:35 AM Sönke Liebau
<soenke.lie...@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.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> <+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
>
