rzo1 commented on PR #2006: URL: https://github.com/apache/stormcrawler/pull/2006#issuecomment-5100581082
Thanks @HarshDevelops for the work here, and @sebastian-nagel for the detailed context. I agree with Sebastian's closing point: the approximate lookup should only happen **when an expected standard header is not found**, rather than unconditionally on every key. I replicated the matching logic from this PR against the real `HttpHeaders` dictionary to see how it behaves in practice. Three things came out of it: **1. Sebastian's `X-Location` example reproduces, and isn't alone:** ``` X-Location -> Location (distance=1, threshold=3) X-Server -> Server (distance=1, threshold=2) ``` Because this hooks into `normalizeKey()`, it fires in `addValue()`, so it is destructive at write time. `X-Location: cached` gets appended into `Location`'s value array, and since `getFirstValue()` returns `values[0]`, which value wins depends on server-controlled header order. In the wrong order we'd follow a redirect to `cached`. **2. The dictionary is a poor fit for crawling.** `org.apache.http.HttpHeaders` gives 55 constants, a good share of them WebDAV (`Dav`, `Depth`, `Destination`, `Overwrite`, `Lock-Token`, `Status-URI`), while missing nearly everything we actually care about: `Set-Cookie`, `Strict-Transport-Security`, `Content-Security-Policy`, `Alt-Svc`, `Link`, `Content-Disposition`, `Access-Control-Allow-Origin`. So we take on fuzzy-match risk for headers nobody sends, and get no help with the ones that matter. **3. There's a performance cost on a path that can't absorb one.** `normalizeKey()` backs all 8 read/write methods on `Metadata`, so it runs for every key, of every tuple, in every bolt. As written, every *miss* (which means all of our internal keys like `fetch.loadingTime` or `protocol.set.cookie`) falls through to a Levenshtein scan across all 55 constants, allocating two `int[]` per comparison. That turns our hottest metadata path from a single `toLowerCase()` into something substantially more expensive, for keys that are never HTTP headers to begin with. Sebastian's lazy/on-miss design happens to fix all three at once: the caller names the header it wants, so it becomes a targeted 1:1 probe instead of a nearest-neighbour search. `X-Location` can never be consulted while `Location` is present. Nothing is merged, the original key survives, and the cost is only paid on a genuine miss. It also lines up with @jnioche's earlier point, and in fact requires it: "is the expected header missing?" is only knowable at the call site, so this can't live inside `Metadata` anyway. ### Suggested way forward Something like `o.a.s.util.HttpHeaderResolver#getFirstValue(Metadata, String)`: exact match -> separator/case-insensitive alias -> fuzzy fallback last, opt-in via config and **off by default**. Then call it from the few places that need a specific header (redirect `Location`, `Content-Type`, `Last-Modified`) rather than globally. One thing worth saying clearly: **the alias half of this PR is the valuable part and carries no risk.** `ContentType`, `contenttype`, `content_type` -> `Content-Type` are all exact dictionary hits that need no Levenshtein at all. That could land on its own, separately from the spellchecking, if you'd like to split it. It would close the useful part of #108 without any of the above. Also, given NUTCH-2563 / NUTCH-3122, Nutch seems to be moving in this same direction, so going the on-miss route keeps us aligned rather than porting a class upstream is walking back. Happy to help review the reworked version. -- 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]
