Copilot commented on code in PR #11826:
URL: https://github.com/apache/ozone/pull/11826#discussion_r4402242991
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/ipc_/Client.java:
##########
@@ -97,6 +97,13 @@ public static void setCallIdAndRetryCount(int cid, int rc) {
retryCount.set(rc);
}
+ /**
+ * Clear a call id set for the next call but never consumed by a {@link
Call}.
+ */
+ public static void clearCallId() {
+ callId.remove();
Review Comment:
`setCallIdAndRetryCount()` sets both `callId` and `retryCount`, but
`clearCallId()` only removes `callId`. If the motivating scenario is a call
that exits before a `Call` is constructed, `retryCount` will also remain set
for the thread, which is inconsistent and can leak stale state into later logic
that consults `retryCount`. Consider clearing both thread-locals (e.g.,
`retryCount.remove()` here as well), or renaming the API to reflect what state
is actually cleared.
##########
hadoop-ozone/common/src/test/java/org/apache/hadoop/ozone/om/ha/TestHadoopRpcOMFollowerReadFailoverProxyProvider.java:
##########
@@ -498,6 +498,22 @@ void testReadException() throws Exception {
assertHandledBy(1);
}
+ @Test
+ void testReadAfterDnsRefreshOfCurrentProxy() throws Exception {
+ setupProxyProvider(3);
+ doRead();
+ assertHandledBy(0);
+
+ // Re-resolving the address after it changed discards the cached proxy of
that node.
+ OMProxyInfo<OzoneManagerProtocolPB> current =
proxyProvider.getCurrentProxy();
+ current.setCachedAddressForTest(new
InetSocketAddress(InetAddress.getByAddress(new byte[] {127, 0, 0, 99}), 0));
Review Comment:
This test uses a hard-coded loopback IP via raw bytes and forces port `0`,
which reads as a magic value and can obscure what aspect of the address change
is being validated (host vs port). Consider using a clearer construction (e.g.,
`InetAddress.getLoopbackAddress()` or a named constant for the IP) and
preserving the original port when the intention is only to simulate a
host/address change. This makes the test’s trigger condition more explicit and
easier to maintain.
##########
hadoop-ozone/common/src/test/java/org/apache/hadoop/ozone/om/ha/TestHadoopRpcOMFollowerReadFailoverProxyProvider.java:
##########
@@ -498,6 +498,22 @@ void testReadException() throws Exception {
assertHandledBy(1);
}
+ @Test
+ void testReadAfterDnsRefreshOfCurrentProxy() throws Exception {
+ setupProxyProvider(3);
+ doRead();
+ assertHandledBy(0);
+
+ // Re-resolving the address after it changed discards the cached proxy of
that node.
Review Comment:
The test name/comment says \"DnsRefresh\", but the test forces an address
mismatch by directly injecting a raw IP/port into `cachedAddressForTest`, which
doesn’t exercise an actual DNS re-resolution. To keep intent clear and reduce
confusion when diagnosing failures, rename the test (and/or adjust the comment)
to reflect what it actually validates (e.g., address refresh invalidates the
cached proxy).
##########
hadoop-ozone/common/src/test/java/org/apache/hadoop/ozone/om/ha/TestHadoopRpcOMFollowerReadFailoverProxyProvider.java:
##########
@@ -498,6 +498,22 @@ void testReadException() throws Exception {
assertHandledBy(1);
}
+ @Test
+ void testReadAfterDnsRefreshOfCurrentProxy() throws Exception {
+ setupProxyProvider(3);
+ doRead();
+ assertHandledBy(0);
+
+ // Re-resolving the address after it changed discards the cached proxy of
that node.
+ OMProxyInfo<OzoneManagerProtocolPB> current =
proxyProvider.getCurrentProxy();
+ current.setCachedAddressForTest(new
InetSocketAddress(InetAddress.getByAddress(new byte[] {127, 0, 0, 99}), 0));
Review Comment:
The test name/comment says \"DnsRefresh\", but the test forces an address
mismatch by directly injecting a raw IP/port into `cachedAddressForTest`, which
doesn’t exercise an actual DNS re-resolution. To keep intent clear and reduce
confusion when diagnosing failures, rename the test (and/or adjust the comment)
to reflect what it actually validates (e.g., address refresh invalidates the
cached proxy).
--
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]
