adoroszlai commented on code in PR #10487:
URL: https://github.com/apache/ozone/pull/10487#discussion_r3419820558
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/scm/proxy/SCMFailoverProxyProviderBase.java:
##########
@@ -260,6 +289,85 @@ public synchronized void close() throws IOException {
}
}
+ /**
+ * Re-resolve the configured hostname for the given SCM nodeId. If DNS
+ * now returns a different IP, swap in a fresh {@link SCMProxyInfo}
+ * (with the new resolved address) and discard any cached proxy so the
+ * next {@link #getProxy()} call dials the new IP.
+ *
+ * @return true when a swap occurred; false when the hostname was not
+ * preserved, the IP is unchanged, the lookup failed, or the
+ * nodeId is unknown.
+ */
+ boolean refreshProxyAddressIfChanged(String nodeId) {
+ // Read the cached info first so we can do the DNS lookup outside
+ // any monitor. A slow / dead resolver while holding the provider
+ // monitor would freeze every concurrent getProxy() / shouldRetry()
+ // caller.
+ String hostAndPort;
+ InetSocketAddress cachedAddress;
+ String serviceId;
+ synchronized (this) {
+ SCMProxyInfo cached = scmProxyInfoMap.get(nodeId);
+ if (cached == null) {
+ return false;
+ }
+ hostAndPort = cached.getHostAndPort();
+ if (hostAndPort == null) {
+ return false;
+ }
+ cachedAddress = cached.getAddress();
+ serviceId = cached.getServiceId();
+ }
Review Comment:
`SCMProxyInfo` is immutable, so the `synchronized` block can be reduced to:
```java
synchronized (this) {
cached = scmProxyInfoMap.get(nodeId);
}
```
and checks and other assignments can be executed afterwards.
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/scm/proxy/SCMFailoverProxyProviderBase.java:
##########
@@ -342,8 +450,24 @@ public RetryAction shouldRetry(Exception e, int retry,
printRetryMessage(e, failover, retryAction.delayMillis);
}
+ // Before advancing the failover index, give the cached SCM
+ // address a chance to be re-resolved -- the same nodeId may
+ // have moved to a new IP under a stable hostname (Kubernetes
+ // pod restart). Limited to connection-class exceptions to
+ // avoid extra DNS load on application-level errors.
+ boolean refreshed = false;
+ if (resolveOnFailureEnabled
+ && ConnectionFailureUtils.isConnectionFailure(e)) {
+ refreshed = refreshProxyAddressIfChanged(getCurrentProxySCMNodeId());
+ }
+
if (SCMHAUtils.checkRetriableWithNoFailoverException(e)) {
setUpdatedLeaderNodeID();
+ } else if (refreshed) {
Review Comment:
nit: swap branches, `if (refreshed)` is cheaper at this point
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/scm/proxy/SCMFailoverProxyProviderBase.java:
##########
@@ -260,6 +289,85 @@ public synchronized void close() throws IOException {
}
}
+ /**
+ * Re-resolve the configured hostname for the given SCM nodeId. If DNS
+ * now returns a different IP, swap in a fresh {@link SCMProxyInfo}
+ * (with the new resolved address) and discard any cached proxy so the
+ * next {@link #getProxy()} call dials the new IP.
+ *
+ * @return true when a swap occurred; false when the hostname was not
+ * preserved, the IP is unchanged, the lookup failed, or the
+ * nodeId is unknown.
+ */
+ boolean refreshProxyAddressIfChanged(String nodeId) {
+ // Read the cached info first so we can do the DNS lookup outside
+ // any monitor. A slow / dead resolver while holding the provider
+ // monitor would freeze every concurrent getProxy() / shouldRetry()
+ // caller.
+ String hostAndPort;
+ InetSocketAddress cachedAddress;
+ String serviceId;
+ synchronized (this) {
+ SCMProxyInfo cached = scmProxyInfoMap.get(nodeId);
+ if (cached == null) {
+ return false;
+ }
+ hostAndPort = cached.getHostAndPort();
+ if (hostAndPort == null) {
+ return false;
+ }
+ cachedAddress = cached.getAddress();
+ serviceId = cached.getServiceId();
+ }
+ InetSocketAddress refreshed;
+ try {
+ refreshed = NetUtils.createSocketAddr(hostAndPort);
+ } catch (IllegalArgumentException ex) {
+ getLogger().warn("Failed to re-resolve SCM address {}",
+ hostAndPort, ex);
+ return false;
+ }
+ if (refreshed.isUnresolved()) {
+ getLogger().warn("SCM hostname {} re-resolved to an unresolved "
+ + "address; leaving cached entry in place.", hostAndPort);
+ return false;
+ }
+ // Null-safe IP comparison. SCMProxyInfo's constructor allows
+ // an unresolved cached address (warns but stores). In that case
+ // cachedAddress.getAddress() is null and a successful
+ // re-resolution is genuinely a change -- proceed to swap rather
+ // than NPE on .equals().
+ java.net.InetAddress cachedIp = cachedAddress.getAddress();
Review Comment:
nit: add import
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
