rzo1 commented on PR #1935:
URL: https://github.com/apache/stormcrawler/pull/1935#issuecomment-4656691177
Thanks for the PR. Two minor, non-blocking points I'd like to raise:
**1. `getConfiguredProxy` relies on `toString()` equality, which is brittle
for the component-key path**
```java
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 `SCProxy` instance for
`LEAST_USED` usage accounting, a miss is harmless (the metadata proxy is still
used, just as a throwaway instance) — so there are no false positives and no
functional bug. But the match is byte-sensitive on `toString()`, and the two
construction paths normalize differently: the component-key path lowercases the
protocol (`type.toLowerCase(Locale.ROOT)`), whereas configured proxies loaded
from the file preserve whatever case the file used (regex group `[^:]+`). So a
configured entry like `HTTP://host:8080` would fail to match a
semantically-identical metadata proxy built from component keys, silently
losing the accounting benefit.
Could we compare on a normalized key, or give `SCProxy` an
`equals()`/`hashCode()`? Also worth noting: the component-key →
configured-match scenario isn't covered by a test (the existing one
deliberately uses an out-of-rotation proxy), so this brittle path is currently
untested.
**2. Port-range validation is duplicated**
The `1..65535` bound now lives in two places with near-identical messages:
```java
// SingleProxyManager.configure() — int from ConfUtils.getInt
if (proxyPort < 1 || proxyPort > 65535) { ... }
```
```java
// ProxyMetadata.validatePort() — String, plus NumberFormatException handling
if (parsedPort < 1 || parsedPort > 65535) { ... }
```
Could we consolidate the bound check into a shared helper (e.g. in
`ProxyMetadata`) that `SingleProxyManager` also calls, keeping the
string-parsing wrapper where it is?
--
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]
