jnioche commented on PR #2052: URL: https://github.com/apache/stormcrawler/pull/2052#issuecomment-5340965406
Thanks @akash-manna-sky **Summary** Genuinely good work. The design is sound, the failure modes are anticipated (self-referencing canonicals, per-page token params, memory bounds, thread safety), the docs are written to the project's standard, and the 47 tests pass and cover the real edge cases (ports, encoding, fragments, relative canonicals, concurrency). Two blockers, a few design points worth settling before merge. **Blockers** 1. Code is not formatted — CI will fail. mvn git-code-format:validate-code-format fails. Running the formatter touches all four Java files (line-width and javadoc reflow, plus DEFAULT_PROTECTED_PARAMS gets exploded one-per-line). The checklist item claims this was done; it wasn't: 2. Two commits, not squashed, and 6 commits behind main. The checklist asks for a single squashed commit rebased on main. **Design points** 3. store is complexity with no payoff. CanonicalRules.java:100 keys a static INSTANCES map by name, and all configuration comes from stormConf — so two stores in the same topology are necessarily identically configured and differ only in which evidence lands where. Meanwhile "the configuration of the first caller wins" is a genuine footgun in local mode / integration tests where several topologies share a JVM, and nothing ever removes entries (cleanup() isn't overridden). I'd either drop store entirely, or put the config in the learner's params and have the URL filter name the learner. As it stands the store knob only buys a hazard. While it's there: "default" is hard-coded in CanonicalParamLearner.java:60 but lives as package-private DEFAULT_STORE in AdaptiveURLNormalizer.java:47 — two literals that must agree. Move it to CanonicalRules as a public constant. 4. Default memory bounds are loose for a broad crawl. max.scopes: 10000 × max.params: 100 allows up to a million ParamStats, each holding two AtomicIntegers and a ConcurrentHashMap-backed set of full path strings. That's a few hundred MB of worker heap in the worst case, and the Caffeine maximumSize on scopes doesn't see it because it counts scopes, not params. Either put a weigher on scopes reflecting the param count, drop max.params to something like 20, or at minimum document the heap implication next to the setting. 5. "Rules are only ever added, never withdrawn" isn't quite true. The comment on promoteIfEstablished says promotions are final, and both javadoc and internals.adoc repeat it — but scopes is a size-bounded Caffeine cache, so an established scope can be evicted and its rules lost. testNumberOfTrackedSitesIsBounded acknowledges this while testAnEstablishedRuleIsNeverWithdrawn asserts the opposite invariant. Not a bug, but the docs should say "unless the host is evicted". -- 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]
