Skip to content

#1951 refactor: replace linear proxy scan with lookup map in MultiProxyManager#1954

Open
leedongkyu0407 wants to merge 1 commit into
apache:mainfrom
leedongkyu0407:fix/multiproxy-lookup-map
Open

#1951 refactor: replace linear proxy scan with lookup map in MultiProxyManager#1954
leedongkyu0407 wants to merge 1 commit into
apache:mainfrom
leedongkyu0407:fix/multiproxy-lookup-map

Conversation

@leedongkyu0407

Copy link
Copy Markdown

Closes #1951

Summary

Replace the O(n) linear scan in getConfiguredProxy() with an O(1) map lookup by putting proxy identity on the type itself.

Changes

  • Added equals/hashCode to SCProxy based on identity fields only: protocol and address (lowercased), port, username, password. Excludes country, area, location, status, and usage.
  • Built a Map<SCProxy, SCProxy> in both configure() overloads and replaced getConfiguredProxy with Optional.ofNullable(map.get(proxy))
  • Removed ProxyUtils.isSameProxy and its private normalize helper, folding the logic into SCProxy.equals

Notes

  • No behavior change
  • Single definition of proxy identity, less dead code
  • MultiProxyManagerTest: 13/13 passed
  • ProtocolTest.testBlocking failure is pre-existing and unrelated to this change

For all changes

  • Is there a issue associated with this PR? Is it referenced in the commit message?
  • Does your PR title start with #XXXX where XXXX is the issue number you are trying to resolve?
  • Has your PR been rebased against the latest commit within the target branch (typically main)?
  • Is your initial contribution a single, squashed commit?
  • Is the code properly formatted with mvn git-code-format:format-code -Dgcf.globPattern="**/*" -Dskip.format.code=false?

For code changes

  • Have you ensured that the full suite of tests is executed via mvn clean verify?
  • Have you written or updated unit tests to verify your changes?
  • If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
  • If applicable, have you updated the LICENSE file, including the main LICENSE file?
  • If applicable, have you updated the NOTICE file, including the main NOTICE file?

@leedongkyu0407 leedongkyu0407 force-pushed the fix/multiproxy-lookup-map branch from ebe2ba1 to 736266b Compare June 16, 2026 09:56
@rzo1

rzo1 commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Improvement] Replace linear proxy scan in MultiProxyManager with a lookup map

2 participants