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 > OpenCore GmbH & Co. KG - Thomas-Mann-Straße 8 - 22880 Wedel - Germany >
