Sounds good. I've added a few sentences to this effect to the KIP. On Thu, Nov 16, 2017 at 5:02 PM, Randall Hauch <rha...@gmail.com> wrote:
> Nice job updating the KIP. The PR ( > https://github.com/apache/kafka/pull/2755/files) for the proposed > implementation does prevent names from being empty, and it trims whitespace > from the name only when creating a new connector. However, the KIP's > "Proposed Change" section should probably be very clear about this, and the > migration section should address how a connector that was created with > leading and/or trailing whitespace characters will still be able to be > updated and deleted. I think that decreases the likelihood of this change > negatively impacting existing users. Basically, going forward, the names of > new connectors will be trimmed. > > WDYT? > > On Thu, Nov 16, 2017 at 9:32 AM, Sönke Liebau < > soenke.lie...@opencore.com.invalid> wrote: > > > I've added some more detail to the KIP [1] around current scenarios that > > might break in the future. I actually came up with a second limitation > that > > we'd impose on users and also documented this. > > > > Let me know what you think. > > > > Kind regards, > > Sönke > > > > [1] > > https://cwiki.apache.org/confluence/display/KAFKA/KIP- > > 212%3A+Enforce+set+of+legal+characters+for+connector+names > > > > > > On Thu, Nov 16, 2017 at 2:59 PM, Sönke Liebau < > soenke.lie...@opencore.com> > > wrote: > > > > > Hi Randall, > > > > > > I had mentioned this edge case in the KIP, but will add some further > > > detail to further clarify all changing scenarios post pull request. > > > > > > Kind regards, > > > Sönke > > > > > > > > > > > > > > > > > > On Thu, Nov 16, 2017 at 2:06 PM, Randall Hauch <rha...@gmail.com> > wrote: > > > > > >> No, we need to keep the KIP since we want to change/correct the > existing > > >> behavior. But we do need to clarify in the KIP these edge cases that > > will > > >> change. > > >> > > >> Thanks for the continued work on this, Sönke. > > >> > > >> Regards, > > >> > > >> Randall > > >> > > >> > On Nov 16, 2017, at 1:56 AM, Sönke Liebau < > soenke.lie...@opencore.com > > >> .INVALID> wrote: > > >> > > > >> > Hi Randall, > > >> > > > >> > zero length definitely works, that's what sent me down this hole in > > the > > >> > first place. I had a customer accidentally create a connector > without > > a > > >> > name in his environment and then be unable to delete it. No > connector > > >> name > > >> > doesn't work, as this throws a null pointer exception due to > > KAFKA-4938 > > >> , > > >> > but once that is fixed would create a connector named "null" I > think. > > >> Have > > >> > not retested this, but seen it in the past. > > >> > > > >> > Also, it is possible to create connectors with trailing and leading > > >> > whitespaces, this errors out on the create request (which will be > > fixed > > >> > when KAFKA-4827 is merged), but correctly creates the connector and > > you > > >> can > > >> > access it if you percent-escape the curl call. This for me is the > main > > >> > reason why a KIP might be needed, as we are changing public facing > > >> behavior > > >> > here. I agree with you, that this will probably not affect anyone or > > >> hardly > > >> > anyone, but in principle it is a change that should need a KIP I > > think. > > >> > > > >> > I've retested and documented this for Confluent 3.3.0: > > >> > https://gist.github.com/soenkeliebau/9363745cff23560fcc234d9b64ac14 > c4 > > >> > > > >> > I am of course happy to withdraw the KIP if you think it is > > unnecessary, > > >> > I've also updated the pull request for KAFKA-4930 to reflect the > > changes > > >> > stated in the KIP and tested the code with Arjuns pull request for > > >> > KAFKA-4827 to ensure they don't interfere with each other. > > >> > > > >> > Let me know what you think. > > >> > > > >> > Kind regards, > > >> > Sönke > > >> > > > >> > ᐧ > > >> > > > >> >> On Tue, Nov 14, 2017 at 7:03 PM, Randall Hauch <rha...@gmail.com> > > >> wrote: > > >> >> > > >> >> Thanks for updating the KIP to reflect the current process. > However, > > I > > >> >> still question whether it is necessary to have a KIP - it depends > on > > >> >> whether it was possible with prior versions to have connectors with > > >> >> zero-length or blank names. Have you tried both of these cases? > > >> >> > > >> >> On Fri, Nov 10, 2017 at 3:52 AM, Sönke Liebau < > > >> >> soenke.lie...@opencore.com.invalid> wrote: > > >> >> > > >> >>> Hi Randall, > > >> >>> > > >> >>> I have set aside some time to work on this next week. The fix > itself > > >> is > > >> >>> quite simple, but I've yet to write tests to properly catch this, > > >> which > > >> >>> turns out to be a bit more complex, as it needs a running > restserver > > >> >> which > > >> >>> is mocked in the tests I've looked at so far. > > >> >>> > > >> >>> Should I withdraw the KIP or update it to reflect the > documentation > > >> >> changes > > >> >>> and enforced rules around trimming and zero length connector > names? > > >> This > > >> >> is > > >> >>> a change to existing behavior, even if it is quite small and > > probably > > >> >> won't > > >> >>> even be noticed by many people.. > > >> >>> > > >> >>> best regards, > > >> >>> Sönke > > >> >>> > > >> >>>> On Thu, Nov 9, 2017 at 9:10 PM, Randall Hauch <rha...@gmail.com> > > >> wrote: > > >> >>>> > > >> >>>> Any progress on updating the PR and withdrawing KIP-212? > > >> >>>> > > >> >>>> On Fri, Oct 27, 2017 at 5:19 PM, Randall Hauch <rha...@gmail.com > > > > >> >> wrote: > > >> >>>> > > >> >>>>> Yes, connector names should not be blank or contain just > > whitespace. > > >> >> In > > >> >>>>> fact, I might recommend that we trim whitespace at the front and > > >> rear > > >> >>> of > > >> >>>>> new connector names and then disallowing any zero-length name. > > >> >> Existing > > >> >>>>> connectors would remain valid, and this would not break backward > > >> >>>>> compatibility. That might require a small kip simply to update > the > > >> >>>>> documentation and specify what names are valid. > > >> >>>>> > > >> >>>>> WDYT? > > >> >>>>> > > >> >>>>> Randall > > >> >>>>> > > >> >>>>> On Fri, Oct 27, 2017 at 1:08 PM, Colin McCabe < > cmcc...@apache.org > > > > > >> >>>> wrote: > > >> >>>>> > > >> >>>>>>> On Wed, Oct 25, 2017, at 01:07, Sönke Liebau wrote: > > >> >>>>>>> I've spent some time looking at this and testing various > > >> >> characters > > >> >>>> and > > >> >>>>>>> it > > >> >>>>>>> would appear that Randall's suspicion was spot on. I think we > > can > > >> >>>>>> support > > >> >>>>>>> a > > >> >>>>>>> fairly large set of characters with very minor changes. > > >> >>>>>>> > > >> >>>>>>> I was put of by the exceptions that were thrown when creating > > >> >>>> connectors > > >> >>>>>>> with certain characters and suspected a larger underlying > > problem > > >> >>> when > > >> >>>>>> in > > >> >>>>>>> fact the only issue is, that the URL in the rest request used > to > > >> >>>>>> retrieve > > >> >>>>>>> the response for the create connector request needs to be > > percent > > >> >>>>>> encoded > > >> >>>>>>> [1]. > > >> >>>>>>> > > >> >>>>>>> I've fixed this and done some local testing which worked out > > quite > > >> >>>>>>> nicely, > > >> >>>>>>> apart from two special cases, I've not been able to find > > >> >> characters > > >> >>>> that > > >> >>>>>>> created issues, even space and slash work. > > >> >>>>>>> The mentioned special cases are: > > >> >>>>>>> \ - if the name contains a backslash that is not the > beginning > > >> >>> of a > > >> >>>>>>> valid escape sequence the request fails before we ever get it > in > > >> >>>>>>> ConnectorsResource, so a backslash would need to be escaped: > \\ > > >> >>>>>>> " - Quotation marks need to be escaped as well to keep the > > json > > >> >>>> body > > >> >>>>>>> of > > >> >>>>>>> the request legal: \" > > >> >>>>>>> In both cases the escape character will be part of the > connector > > >> >>> name > > >> >>>>>> and > > >> >>>>>>> need to be specified in the url to retrieve the connector as > > well, > > >> >>>> even > > >> >>>>>>> though we could URL encode it in a legal way without escaping > > >> >> here. > > >> >>> So > > >> >>>>>>> they > > >> >>>>>>> work, not sure if I'd recommend using those characters, but no > > >> >> real > > >> >>>>>>> reason > > >> >>>>>>> to prohibit people from using them that I can see either. > > >> >>>>>> > > >> >>>>>> Good research, Sönke. > > >> >>>>>> > > >> >>>>>>> > > >> >>>>>>> > > >> >>>>>>> What I'd do going forward is: > > >> >>>>>>> - withdraw the KIP, as I don't see a real need for one, since > > this > > >> >>> is > > >> >>>>>> not > > >> >>>>>>> changing anything, just fixing things. > > >> >>>>>>> - add a section to the documentation around legal characters, > > >> >>> specify > > >> >>>>>> the > > >> >>>>>>> ones I tested explicitly (url encoded %20 - %7F) and mention > > that > > >> >>> most > > >> >>>>>>> other characters should work as well but no guarantees are > given > > >> >>>>>>> - update the pull request for KAFKA-4930 to allow all > characters > > >> >> but > > >> >>>>>>> still > > >> >>>>>>> prohibit creating a connector with an empty name. I'd propose > to > > >> >>> keep > > >> >>>>>> the > > >> >>>>>>> validator though as it'll give us a central location to do any > > >> >>>> checking > > >> >>>>>>> that might turn out to be necessary later on. > > >> >>>>>> > > >> >>>>>> Are empty names currently allowed? That's unfortunate. > > >> >>>>>> > > >> >>>>>>> - add some integration tests to check connectors with special > > >> >>>> characters > > >> >>>>>>> in > > >> >>>>>>> their names work > > >> >>>>>>> - fix the url encoding line in ConnectorsResource > > >> >>>>>>> > > >> >>>>>>> Does that sound fair to everybody? > > >> >>>>>> > > >> >>>>>> It sounds good to me, but I will let someone more knowledgeable > > >> >> about > > >> >>>>>> connect chime in. > > >> >>>>>> > > >> >>>>>> best, > > >> >>>>>> Colin > > >> >>>>>> > > >> >>>>>>> > > >> >>>>>>> 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#L102 > > >> >>>>>>> > > >> >>>>>>> On Tue, Oct 24, 2017 at 8:40 PM, Colin McCabe < > > cmcc...@apache.org > > >> >>> > > >> >>>>>> wrote: > > >> >>>>>>> > > >> >>>>>>>>> On Tue, Oct 24, 2017, at 11:28, Sönke Liebau wrote: > > >> >>>>>>>>> Hi, > > >> >>>>>>>>> > > >> >>>>>>>>> after reading your messages I'll grant that I might have > > >> >> picked > > >> >>> a > > >> >>>>>>>>> somewhat > > >> >>>>>>>>> draconic option to solve these issues. > > >> >>>>>>>>> > > >> >>>>>>>>> In general I believe that properly encoding the URLs after > > >> >>> having > > >> >>>>>> created > > >> >>>>>>>>> the connectors should solve a lot of the issues already. For > > >> >>> some > > >> >>>>>>>>> characters the rest api returns an error on creating the > > >> >>> connector > > >> >>>>>> as > > >> >>>>>>>>> well, > > >> >>>>>>>>> so for that URL encoding won't help. However the connectors > do > > >> >>> get > > >> >>>>>>>>> created > > >> >>>>>>>>> even though an error is returned, I've never investigated if > > >> >>> they > > >> >>>>>> are in > > >> >>>>>>>>> a > > >> >>>>>>>>> consistent state tbh - I'll give this another look. > > >> >>>>>>>>> > > >> >>>>>>>>> @colin: Entity encoding would allow us to encode a lot of > > >> >>>>>> characters, > > >> >>>>>>>>> however I am unsure whether we should prefer it over url > > >> >>> encoding > > >> >>>>>> in this > > >> >>>>>>>>> case, as mostly the end user would have to encode the > > >> >> characters > > >> >>>>>> himself. > > >> >>>>>>>>> And due to entity encoding ending every character with a ; > > >> >> which > > >> >>>>>> causes > > >> >>>>>>>>> the > > >> >>>>>>>>> embedded jetty server to cut the connector name at that > > >> >>> character > > >> >>>>>> we'd > > >> >>>>>>>>> probably need to encode that character in URL encoding again > > >> >> for > > >> >>>>>> that to > > >> >>>>>>>>> work out - which might get a bit too complex tbh. > > >> >>>>>>>> > > >> >>>>>>>> Sorry, I meant to write percent-encoding, not entity refs. > > >> >>>>>>>> https://en.wikipedia.org/wiki/Percent-encoding > > >> >>>>>>>> > > >> >>>>>>>> best, > > >> >>>>>>>> Colin > > >> >>>>>>>> > > >> >>>>>>>> > > >> >>>>>>>>> I will further investigate which characters the url decoding > > >> >>> that > > >> >>>>>> jetty > > >> >>>>>>>>> brings to the table will let us use and if all of these are > > >> >>>>>> correctly > > >> >>>>>>>>> handled during connector creation and report back with a new > > >> >>> list > > >> >>>> of > > >> >>>>>>>>> characters that I think we can support fairly easily. > > >> >>>>>>>>> > > >> >>>>>>>>> Kind regards, > > >> >>>>>>>>> Sönke > > >> >>>>>>>>> > > >> >>>>>>>>> > > >> >>>>>>>>> On Tue, Oct 24, 2017 at 6:42 PM, Colin McCabe < > > >> >>> cmcc...@apache.org > > >> >>>>> > > >> >>>>>>>> wrote: > > >> >>>>>>>>> > > >> >>>>>>>>>> It should be possible to use entity references to encode > > >> >> these > > >> >>>>>>>>>> characters in URLs. See https://dev.w3.org/html5/html- > > >> >>>>>> author/charref > > >> >>>>>>>>>> Maybe I'm misunderstanding the problem, but can we simply > > >> >>> encode > > >> >>>>>> the > > >> >>>>>>>>>> URLs, rather than restricting the names? > > >> >>>>>>>>>> > > >> >>>>>>>>>> best, > > >> >>>>>>>>>> Colin > > >> >>>>>>>>>> > > >> >>>>>>>>>> > > >> >>>>>>>>>>> On Mon, Oct 23, 2017, at 14:12, Randall Hauch wrote: > > >> >>>>>>>>>>> Here's the link to KIP-212: > > >> >>>>>>>>>>> https://cwiki.apache.org/confluence/pages/viewpage. > > >> >>>>>>>>>> action?pageId=74684586 > > >> >>>>>>>>>>> > > >> >>>>>>>>>>> I do think it's worthwhile to define the rules for > > >> >> connector > > >> >>>>>> names. > > >> >>>>>>>>>>> However, I think it would be better to describe the > > >> >> current > > >> >>>>>>>> restrictions > > >> >>>>>>>>>>> for names outside of them appearing within URLs. For > > >> >>> example, > > >> >>>>>> if we > > >> >>>>>>>> can > > >> >>>>>>>>>>> keep connector names relatively free of constraints but > > >> >>>> instead > > >> >>>>>>>> define > > >> >>>>>>>>>>> how > > >> >>>>>>>>>>> names should be encoded when used within URLs (e.g., URL > > >> >>>>>> encoding), > > >> >>>>>>>> then > > >> >>>>>>>>>>> we > > >> >>>>>>>>>>> may not have (m)any backward compatibility issues other > > >> >> than > > >> >>>>>> fixing > > >> >>>>>>>> some > > >> >>>>>>>>>>> bugs related to proper encoding/decoding. > > >> >>>>>>>>>>> > > >> >>>>>>>>>>> Thoughts? > > >> >>>>>>>>>>> > > >> >>>>>>>>>>> > > >> >>>>>>>>>>> On Mon, Oct 23, 2017 at 3:44 PM, Sönke Liebau < > > >> >>>>>>>>>>> soenke.lie...@opencore.com.invalid> wrote: > > >> >>>>>>>>>>> > > >> >>>>>>>>>>>> All, > > >> >>>>>>>>>>>> > > >> >>>>>>>>>>>> I've created a KIP to discuss enforcing of rules on what > > >> >>>>>>>> characters are > > >> >>>>>>>>>>>> allowed in connector names. > > >> >>>>>>>>>>>> > > >> >>>>>>>>>>>> Since this may break api calls that are currently > > >> >> working > > >> >>> I > > >> >>>>>>>> figured a > > >> >>>>>>>>>> KIP > > >> >>>>>>>>>>>> is the better way to go than to just create a jira. > > >> >>>>>>>>>>>> > > >> >>>>>>>>>>>> I'd love to hear your input on this! > > >> >>>>>>>>>>>> > > >> >>>>>>>>>> > > >> >>>>>>>>> > > >> >>>>>>>>> > > >> >>>>>>>>> > > >> >>>>>>>>> -- > > >> >>>>>>>>> Sönke Liebau > > >> >>>>>>>>> Partner > > >> >>>>>>>>> Tel. +49 179 7940878 > > >> >>>>>>>>> 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 > > >> >>>>>> > > >> >>>>> > > >> >>>>> > > >> >>>> > > >> >>> > > >> >>> > > >> >>> > > >> >>> -- > > >> >>> 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 <+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 > > > -- Sönke Liebau Partner Tel. +49 179 7940878 OpenCore GmbH & Co. KG - Thomas-Mann-Straße 8 - 22880 Wedel - Germany
