rzo1 commented on PR #1954: URL: https://github.com/apache/stormcrawler/pull/1954#issuecomment-4717250826
Thanks for the refactor, the O(n) to O(1) lookup and folding proxy identity into `SCProxy.equals`/`hashCode` looks correct and consistent with the removed `isSameProxy`. Two things I'd like to see addressed before merge: **1. Missing tests for the new `equals`/`hashCode`.** The identity contract is now the heart of the lookup, but it has no coverage. Could you add unit tests covering: - equal on identical identity fields, and `hashCode` equal for equal objects - case-insensitivity of `protocol`/`address` - not-equal on differing `port` / `username` / `password` - `country` / `area` / `location` / `status` are excluded from identity - null / different-type handling - a `MultiProxyManagerTest` case confirming metadata-driven lookup resolves to the configured instance through the new map (the path that replaced the linear scan) **2. Mutable fields used in the hash key.** `username` and `password` are non-final, yet they are part of `equals`/`hashCode` and these objects are used as `HashMap` keys. It is safe today because they are only assigned in the constructor, but mutating either after insertion would corrupt the map lookup. Could you either make them `final` (preferred, they are never reassigned after construction) or add a short comment noting the invariant? Minor: the diff also carries unrelated reformatting in `MultiProxyManager.java` (Javadoc reflow, reduced continuation indents, blank line after imports) that does not match the project's `git-code-format` style and will fail the format check. Running `mvn git-code-format:format-code -Dgcf.globPattern="**/*" -Dskip.format.code=false` should bring the diff down to just the real change. -- 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]
