jnioche commented on PR #2052:
URL: https://github.com/apache/stormcrawler/pull/2052#issuecomment-5315368549
thanks @akash-manna-sky
a quick AI assisted review
Thanks for tackling #315 — the PR hygiene here is good (formatting, unit
tests, docs, issue linkage all present). Most of my comments are about how the
filter interacts with the topol
ogy, which is hard to see without tracing through several bolts.
## Blocker: the filter can't learn anything in a standard topology
`learn()` reads the canonical from `sourceMetadata`, but in both parsing
bolts the URL filters are applied to the outlinks **before** the parse filters
that produce the canonical:
* `JSoupParserBolt`: `toOutlinks(url, metadata, slinks)` (line 452) →
`filterOutlink` → `urlFilters.filter` (line 604), whereas `jsoupFilters.filter`
is line 464 and `parseFilters.fil
ter` is line 478.
* `external/tika` `ParserBolt`: outlink filtering at line 406 vs
`parseFilters.filter` at line 282.
`canonical` is extracted by `XPathFilter`, a parse filter. So at filtering
time `sourceMetadata.getFirstValue("canonical")` is always null and `learn()`
returns early. The unit tests
pass because they inject the metadata by hand.
The feature only does anything if the user adds `canonical` to
`metadata.persist` (so it is learnt from the *previous* fetch), or places the
filtering inside `LinkParseFilter` ordered
after the XPath filter — neither of which is documented. The docs hunk says
the filter "must therefore be placed in a parsing bolt", which isn't sufficient.
This needs a design decision rather than a patch: either learn from
somewhere that runs after the parse filters, or explicitly require `canonical`
in `metadata.persist` and document t
hat rules are learnt one fetch cycle late.
## Also structural
**Thread safety.** `stats.get(k, ...)` returns a plain `HashMap` that is
mutated outside any lock, plus non-volatile `int` counters and
`lastLearnedSource`. `URLFilter` instances *are
* used concurrently: `StatusEmitterBolt.urlFilters` is a single instance
shared by all `FetcherThread`s, and `FetcherBolt` calls `emitOutlink` from
those threads (lines 635 and 857).
Every other built-in filter is stateless, so this contract has never been
exercised. Two fetcher threads handling redirects for the same host can corrupt
the scope map or lose counter
increments. `ConcurrentHashMap` + `AtomicInteger`/`LongAdder` would cover
it.
**Non-deterministic normalisation.** The learned evidence is per-instance,
and `URLFilters.fromConf` builds a fresh instance per bolt
(`URLFilters.java:86`) — one per parser task, one
per fetcher, one in `URLFilterBolt` (which calls `filter(null, tempMed,
url)` and so never learns), plus a second one inside `LinkParseFilter`.
`urlfilters.config.file` is global, so
there's no way to load the filter only in the parsing bolt as the docs
suggest. The same outlink then gets emitted as `?id=1&sid=x` by one task and
`?id=1` by another, so both forms
land in the status store and both keep being fetched — the opposite of the
intended dedup. URLs discovered before a rule was learnt are never reconciled
either.
**Content-bearing parameters can be learnt.** Self-referencing canonicals
are a common misconfiguration: plenty of sites serve `/list?page=2..N` with
`<link rel="canonical" href="/lis
t">`. `sameResource()` accepts that (same host/port/path), so after
`minObservations` such pages `page` hits a 1.0 drop ratio and becomes
removable. Since the rule is applied to outli
nks before they're emitted as DISCOVERED, `?page=2..N` are all rewritten to
`/list`, deduplicated away and never fetched — the crawl silently loses all
paginated content, with no evid
ence decay and no recovery path. A denylist of protected parameter names
(`page`, `p`, `offset`, `start`, `q`, …), or requiring the canonical to drop
the parameter across several dist
inct paths, would limit the blast radius.
## Smaller things
* **`minObservations` counts observations, not distinct pages.**
`lastLearnedSource` only suppresses *consecutive* re-observations of the same
source. With `fetchInterval.default` at
1440 min, a host whose only query-string URL is `/x?sid=1` accumulates
`dropped=5, total=5` over five days and the rule is applied on the strength of
one page. The docs say "number of
pages a parameter must have been seen on" — nothing tracks distinct source
URLs.
* **URL reconstruction changes unrelated parts, and only when something was
removed.** `url.getPath()` is `""` for `http://example.com?sid=1&id=2`, so the
result is `http://example.co
m?id=2` — a different status-store key from the `http://example.com/?id=2`
that `BasicURLNormaliser` produces. Similarly, if `URLUtil.toURL` had to
sanitize the input (space, `|`, `\`
, `{`, `}`), the sanitized form is returned, so `/a b?sid=1&id=2` →
`/a%20b?out a removable parameter is returned verbatim. Appending `/` for an
empty pat
h and rebuilding only the query portion of the original string would avoid
both.
* **`maxParams` has no eviction.** The size check counts every parameter
evevant ones, and entries are never removed. A site with per-page tokens in the
p
arameter *name* (cache-busters, `utm_term_<hash>`, CMS facets like
`f[123]=`) fills the 100 slots with single-observation entries early in the
crawl, after which the genuinely removab
le parameter can never be tracked for that host. A size-bounded cache (Caffe
or dropping single-observation entries when full, would be self-healing.
* **`testNumberOfTrackedSitesIsBounded` doesn't exercise eviction.** It sets
`maxScopes: 1` but only ever observes `example.com`, so the bound is never
reached. Observing two distinct
hosts and asserting the first host's evidence was discarded would actually
Things that do look correct: null handling, `MalformedURLException` handling
correctly leaves `mailto:`/`file:` URLs alone), default-port comparison, and
the raw-vs-decoded parameter-name handling.
--
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]
