Very late to the game here, but a few thoughts: 1. Regarding whether KIP is necessary, I don't mind doing it for documentation sake, but I would classify any mishandling of connector names here as a bug. Which doesn't require a KIP to fix.
2. For support of characters, Kafka has some history of just being restrictive (e.g., see topic name restrictions), but I personally disagree with this approach. I think it is better to be liberal in what we accept and just document limitations. I think our default should be to accept any user input and document why we can't handle certain inputs and how the user should adapt if we can't. In general I try to work under the robustness principle: *Be conservative in what you do, be liberal in what you accept from others* 3. Related to 2, there were some cases like whitespace-only connector names. This seems extremely weird and not critical, so I'm fine not supporting it officially, but technically I don't see any reason it shouldn't be supported with any appropriate escaping (i.e. what would it break for us?). But in general, I think just being more explicit about expectations is great and it'd be great to set baseline expectations. -Ewen On Mon, Nov 20, 2017 at 12:33 AM, Sönke Liebau < soenke.lie...@opencore.com.invalid> wrote: > @Randall: are you happy with the KIP as it stands so I can call for a vote, > or are there any outstanding items still to discuss? > > Same question to anybody else who'd like to participate of course :) > > On Thu, Nov 16, 2017 at 5:35 PM, Sönke Liebau <soenke.lie...@opencore.com> > wrote: > > > 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/9363745cff23560fcc234d9 > >> b64ac14c4 > >> > >> > > >> > >> > 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 <+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 >
