rzo1 commented on PR #1846: URL: https://github.com/apache/stormcrawler/pull/1846#issuecomment-4194020718
> Can we have tests for the new classes at all? Sure, will add some tests. > Should some of them be in Apache Storm instead as they are not specific to SC That's a fair point worth addressing directly. Thinking out loud, i.e. what could theoretically live upstream: `ScopedCounter`, `ScopedReducedMetric`, `DualCounterMetric`, and `DualReducedMetric` are thin abstractions over Storm's own `MultiCountMetric`/`MultiReducedMetric`. In principle, Storm could offer these as first-class interfaces to ease V1→V2 migration for any Storm user, not just SC. From my POV, the real value in this PR is not the interfaces themselves but it's `CrawlerMetrics`, the factory that routes to V1, V2, or both based on configuration. That class has hard dependencies on SC-specific types: - PerSecondReducer (SC core) - CollectionMetric (SC core) - ConfUtils (SC core) If we upstreamed only the interfaces to Storm, `CrawlerMetrics` would still have to live in SC anyway. The interfaces without the factory aren't particularly useful on their own; they'd just be empty contracts with no wiring. There's also a timing argument: Storm already ships a V2 metrics API. What it's missing is not these wrapper interfaces, but a first-class migration path for existing V1 users. That's a larger design discussion for the (rather inactive) Storm community, and tying this PR to that would block a useful, self-contained improvement to SC indefinitely. If Storm eventually adopts a similar abstraction, we could migrate `CrawlerMetrics` to use it and deprecate the SC-local interfaces at that point. That's a clean upgrade path. For now, keeping everything here would be a pragmatic choice as the interfaces are small, the coupling to Storm internals is intentionally light, and the alternative is a cross-project dependency that does not exist yet. -- 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]
