1143 Add per-URL proxy metadata selection#1935
Conversation
Reference apache#1143. AI-assistant: Codex CLI
|
Thanks for the PR. Two minor, non-blocking points I'd like to raise: 1. private Optional<SCProxy> getConfiguredProxy(SCProxy proxy) {
String proxyString = proxy.toString();
for (SCProxy configuredProxy : this.proxies) {
if (configuredProxy.toString().equals(proxyString)) {
return Optional.of(configuredProxy);
}
}
return Optional.empty();
}Since this only exists to reuse the configured Could we compare on a normalized key, or give 2. Port-range validation is duplicated The // SingleProxyManager.configure() — int from ConfUtils.getInt
if (proxyPort < 1 || proxyPort > 65535) { ... }// ProxyMetadata.validatePort() — String, plus NumberFormatException handling
if (parsedPort < 1 || parsedPort > 65535) { ... }Could we consolidate the bound check into a shared helper (e.g. in |
|
Thanks for the careful review. I pushed a small follow-up for both points:
Local verification: git diff --check
/tmp/apache-maven-3.9.9/bin/mvn -pl core -Dtest=MultiProxyManagerTest,SingleProxyManagerTest testThe focused Maven run passed: 30 tests, 0 failures/errors. |
dpol1
left a comment
There was a problem hiding this comment.
+1 from my side. Good coverage, including the edge cases for partial auth, out-of-range ports, and credentials with reserved characters. Fail-fast on invalid metadata is the right call.
Minor note for a follow-up: getConfiguredProxy in MultiProxyManager does a linear scan per metadata-override request. With large proxy lists a HashMap keyed on normalized protocol+host+port in configure() would avoid the per-request work. Not a blocker here — @rzo1 worth an issue?
Fixes #1143.
Summary
http.proxy.skip=true, fullhttp.proxyconnection strings, and component metadata keys (http.proxy.host,http.proxy.port,http.proxy.type,http.proxy.user,http.proxy.pass).Verification
git diff --checkmvn -B -ntp -Dskip.format.code=false -pl core git-code-format:format-code -DskipTestsmvn -B -ntp -pl core -Dtest=SingleProxyManagerTest,MultiProxyManagerTest,HttpProtocolProxyConcurrencyTest,HttpClientProtocolProxyManagerTest,FetcherBoltTest,SimpleFetcherBoltTest -DfailIfNoTests=false testFocused selector result:
Tests run: 44, Failures: 0, Errors: 0, Skipped: 0,BUILD SUCCESS.Note: I also attempted Docker
mvn clean verify. Thecoremodule passed 264 tests and the reactor advanced pastcore, but the full reactor later failed instormcrawler-opensearchon JaCoCo threshold failures unrelated to this patch, so I am not claiming a full-repo pass.