abhinav-phi commented on issue #2145: URL: https://github.com/apache/stormcrawler/issues/2145#issuecomment-6225218181
@omarbounawarapy option 1. and i need to correct something i told you earlier rather than let it look like i changed the subject. i said `getCanonicalHost` is the right place and that is still where the collapsing belongs, in one function rather than scattered. what i did not think about is that `normaliseHost` calls it at `URLUtil.java:335` and `AbstractStatusUpdaterBolt` writes that result into the status store behind `status.updater.normalise.hosts`. so punycoding inside `getCanonicalHost` does not just change a key, it silently starts rewriting stored urls, which is the exact downside you described in option 2. the method also has the other contract written into it, the comment above the decoder says okhttp percent-decodes the host when it parses the url and the keys should agree with what it connects to, and `percentDecodeHost` spells out that the octets are read as utf-8 so `%C3%BC` becomes one `ü` rather than one char per octet. your test failure is the test doing its job, leave it alone. your normalizer argument is the stronger one and i checked it. `BasicURLNormalizer` already does `IDN.toASCII` behind `hostIDNtoASCII`, the field is false by default and all three archetypes switch it on, and it catches the failure instead of throwing. rewriting the stored url to punycode already has an owner, so the status updater doing it regardless of that switch is a behaviour change nobody asked for. one thing to fix in the plan before you write it, the fan out is larger than the two keys i named. `getCanonicalHost` has five callers and all five are key paths, so the ascii step has to land on each of them or one server ends up with a unicode key in one place and a punycode key in another: ``` fetcher/FetchItem.java:73 bolt/SimpleFetcherBolt.java:634 bolt/URLPartitionerBolt.java:90 util/URLPartitioner.java:64 protocol/HttpRobotRulesParser.java:115 ``` i would not write `IDN.toASCII` five times either. a second helper beside it, something like `getASCIICanonicalHost(URL)` that decodes, lowercases, drops the root label and then ascii folds, keeps the two forms obviously related and gives the test one target. do not reuse `URLUtil.toASCII` for this, it does exist and does `IDN.toASCII(u.getHost())`, but it reads the raw host so it neither percent-decodes nor strips the trailing dot, which is the entire point of the canonical form. on the `URLPartitioner` churn you flagged, that is real and i am not going to pretend it is free. the key string changes, so the same host lands on a different partition before and after, and for a running topology that is a window where one server gets fetched under two queue ids until the old keys drain. i do not think it blocks the fix, robots.txt correctness matters more, but it belongs in the pr description rather than buried in a commit, and @rzo1 should get to say whether they care about the window. two cases worth pinning now that the step lives in one place. `http://examplé.org/`, `http://ExAmplé.org/` and `http://xn--exampl-gva.org/` all giving the same key. you already proved the third one, the other two follow from `IDN.toASCII` doing nameprep which folds case, but i have not run java on this machine so treat that as something for the test to settle rather than a claim from me. and a host that is already plain ascii, `example.com`, coming back byte for byte identical so keys already stored stay stable on a re-read. go ahead and open it, and thanks for pushing back on the test instead of deleting the assertion. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
