@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
