Addressed next set of review feedback:

- expose only DisconnectionHistory instead of ConnectionTarget as protected
- Nullable annotation on ConnectionTarget's DisconnectionHistory field
- log seconds and milliseconds since last connect, as #s/#ms
- in test, re-use thread pool, narrowed try-block for log checking, used default reconnection period, and updated logs to test for time formatting

Author: schase-es

server/src/main/java/org/elasticsearch/cluster/NodeConnectionsService.java

@@ -215,10 +215,14 @@ public void reconnectToNodes(DiscoveryNodes discoveryNodes, Runnable onCompletio
     }
 
     // exposed for testing
-    protected ConnectionTarget connectionTargetForNode(DiscoveryNode node) {
+    protected DisconnectionHistory disconnectionHistoryForNode(DiscoveryNode node) {
         synchronized (mutex) {
-            return targetsByNode.get(node);
+            ConnectionTarget connectionTarget = targetsByNode.get(node);
+            if (connectionTarget != null) {
+                return connectionTarget.disconnectionHistory;
+            }
         }
+        return null;
     }
 
     /**
@@ -235,14 +239,15 @@ public Exception getDisconnectCause() {
         }
     }
 
-    protected class ConnectionTarget {
+    private class ConnectionTarget {
         private final DiscoveryNode discoveryNode;
 
         private final AtomicInteger consecutiveFailureCount = new AtomicInteger();
         private final AtomicReference<Releasable> connectionRef = new AtomicReference<>();
 
         // access is synchronized by the service mutex
-        protected DisconnectionHistory disconnectionHistory = null;
+        @Nullable // null when node is connected or initialized; non-null in between disconnects and connects
+        private DisconnectionHistory disconnectionHistory = null;
 
         // all access to these fields is synchronized
         private List<Releasable> pendingRefs;
@@ -397,15 +402,17 @@ public void onNodeConnected(DiscoveryNode node, Transport.Connection connection)
 
             if (disconnectionHistory != null) {
                 long millisSinceDisconnect = threadPool.absoluteTimeInMillis() - disconnectionHistory.disconnectTimeMillis;
+                long secondsSinceDisconnect = millisSinceDisconnect / 1000;
                 if (disconnectionHistory.disconnectCause != null) {
                     logger.warn(
                         () -> format(
                             """
                                 reopened transport connection to node [%s] \
-                                which disconnected exceptionally [%dms] ago but did not \
+                                which disconnected exceptionally [%ds/%dms] ago but did not \
                                 restart, so the disconnection is unexpected; \
                                 see [%s] for troubleshooting guidance""",
                             node.descriptionWithoutAttributes(),
+                            secondsSinceDisconnect,
                             millisSinceDisconnect,
                             ReferenceDocs.NETWORK_DISCONNECT_TROUBLESHOOTING
                         ),
@@ -415,10 +422,11 @@ public void onNodeConnected(DiscoveryNode node, Transport.Connection connection)
                     logger.warn(
                         """
                             reopened transport connection to node [{}] \
-                            which disconnected gracefully [{}ms] ago but did not \
+                            which disconnected gracefully [{}s/{}ms] ago but did not \
                             restart, so the disconnection is unexpected; \
                             see [{}] for troubleshooting guidance""",
                         node.descriptionWithoutAttributes(),
+                        secondsSinceDisconnect,
                         millisSinceDisconnect,
                         ReferenceDocs.NETWORK_DISCONNECT_TROUBLESHOOTING
                     );

server/src/test/java/org/elasticsearch/cluster/NodeConnectionsServiceTests.java

@@ -17,7 +17,7 @@
 import org.elasticsearch.action.ActionRunnable;
 import org.elasticsearch.action.support.PlainActionFuture;
 import org.elasticsearch.action.support.SubscribableListener;
-import org.elasticsearch.cluster.NodeConnectionsService.ConnectionTarget;
+import org.elasticsearch.cluster.NodeConnectionsService.DisconnectionHistory;
 import org.elasticsearch.cluster.node.DiscoveryNode;
 import org.elasticsearch.cluster.node.DiscoveryNodeRole;
 import org.elasticsearch.cluster.node.DiscoveryNodeUtils;
@@ -250,18 +250,17 @@ public String toString() {
     }
 
     public void testDisconnectionHistory() {
-        final Settings.Builder settings = Settings.builder();
-        settings.put(CLUSTER_NODE_RECONNECT_INTERVAL_SETTING.getKey(), "100ms");
-
         final DeterministicTaskQueue deterministicTaskQueue = new DeterministicTaskQueue();
         final ThreadPool threadPool = deterministicTaskQueue.getThreadPool();
+        final long reconnectIntervalMillis = CLUSTER_NODE_RECONNECT_INTERVAL_SETTING.get(Settings.EMPTY).millis();
+        final long reconnectIntervalSeconds = reconnectIntervalMillis / 1000;
 
-        MockTransport transport = new MockTransport(deterministicTaskQueue.getThreadPool());
-        TestTransportService transportService = new TestTransportService(transport, deterministicTaskQueue.getThreadPool());
+        MockTransport transport = new MockTransport(threadPool);
+        TestTransportService transportService = new TestTransportService(transport, threadPool);
         transportService.start();
         transportService.acceptIncomingRequests();
 
-        final NodeConnectionsService service = new NodeConnectionsService(settings.build(), threadPool, transportService);
+        final NodeConnectionsService service = new NodeConnectionsService(Settings.EMPTY, threadPool, transportService);
         service.start();
 
         final DiscoveryNode noClose = DiscoveryNodeUtils.create("noClose");
@@ -281,6 +280,14 @@ public void testDisconnectionHistory() {
         assertNullDisconnectionHistory(service, gracefulClose);
         assertNullDisconnectionHistory(service, exceptionalClose);
 
+        transportService.disconnectFromNode(gracefulClose);
+        transportService.disconnectFromNode(exceptionalClose);
+
+        // check disconnection history set after close
+        assertNullDisconnectionHistory(service, noClose);
+        assertDisconnectionHistoryDetails(service, threadPool, gracefulClose, null);
+        assertDisconnectionHistoryDetails(service, threadPool, exceptionalClose, RuntimeException.class);
+
         try (var mockLog = MockLog.capture(NodeConnectionsService.class)) {
             mockLog.addExpectation(
                 new MockLog.SeenEventExpectation(
@@ -289,7 +296,7 @@ public void testDisconnectionHistory() {
                     Level.WARN,
                     "reopened transport connection to node ["
                         + gracefulClose.descriptionWithoutAttributes()
-                        + "] which disconnected gracefully [*ms] ago "
+                        + "] which disconnected gracefully [" + reconnectIntervalSeconds + "s/" + reconnectIntervalMillis + "ms] ago "
                         + "but did not restart, so the disconnection is unexpected; "
                         + "see [https://www.elastic.co/docs/*] for troubleshooting guidance"
                 )
@@ -301,33 +308,24 @@ public void testDisconnectionHistory() {
                     Level.WARN,
                     "reopened transport connection to node ["
                         + exceptionalClose.descriptionWithoutAttributes()
-                        + "] which disconnected exceptionally [*ms] ago "
+                        + "] which disconnected exceptionally [" + reconnectIntervalSeconds + "s/" + reconnectIntervalMillis + "ms] ago "
                         + "but did not restart, so the disconnection is unexpected; "
                         + "see [https://www.elastic.co/docs/*] for troubleshooting guidance"
                 )
             );
-            transportService.disconnectFromNode(gracefulClose);
-            transportService.disconnectFromNode(exceptionalClose);
-
-            // check disconnection history set after close
-            assertNullDisconnectionHistory(service, noClose);
-            assertDisconnectionHistoryDetails(service, threadPool, gracefulClose, null);
-            assertDisconnectionHistoryDetails(service, threadPool, exceptionalClose, RuntimeException.class);
-
-            runTasksUntil(deterministicTaskQueue, 200);
-
-            // check on reconnect -- disconnection history is reset
-            assertNullDisconnectionHistory(service, noClose);
-            assertNullDisconnectionHistory(service, gracefulClose);
-            assertNullDisconnectionHistory(service, exceptionalClose);
-
+            runTasksUntil(deterministicTaskQueue, deterministicTaskQueue.getCurrentTimeMillis() + reconnectIntervalMillis);
             mockLog.assertAllExpectationsMatched();
         }
+
+        // check on reconnect -- disconnection history is reset
+        assertNullDisconnectionHistory(service, noClose);
+        assertNullDisconnectionHistory(service, gracefulClose);
+        assertNullDisconnectionHistory(service, exceptionalClose);
     }
 
     private void assertNullDisconnectionHistory(NodeConnectionsService service, DiscoveryNode node) {
-        ConnectionTarget nodeTarget = service.connectionTargetForNode(node);
-        assertNull(nodeTarget.disconnectionHistory);
+        DisconnectionHistory disconnectionHistory = service.disconnectionHistoryForNode(node);
+        assertNull(disconnectionHistory);
     }
 
     private void assertDisconnectionHistoryDetails(
@@ -336,14 +334,14 @@ private void assertDisconnectionHistoryDetails(
         DiscoveryNode node,
         @Nullable Class<?> disconnectCauseClass
     ) {
-        ConnectionTarget nodeTarget = service.connectionTargetForNode(node);
-        assertNotNull(nodeTarget.disconnectionHistory);
-        assertTrue(threadPool.absoluteTimeInMillis() - nodeTarget.disconnectionHistory.getDisconnectTimeMillis() >= 0);
-        assertTrue(threadPool.absoluteTimeInMillis() - nodeTarget.disconnectionHistory.getDisconnectTimeMillis() <= 200);
+        DisconnectionHistory disconnectionHistory = service.disconnectionHistoryForNode(node);
+        assertNotNull(disconnectionHistory);
+        assertTrue(threadPool.absoluteTimeInMillis() - disconnectionHistory.getDisconnectTimeMillis() >= 0);
+        assertTrue(threadPool.absoluteTimeInMillis() - disconnectionHistory.getDisconnectTimeMillis() <= 200);
         if (disconnectCauseClass != null) {
-            assertThat(nodeTarget.disconnectionHistory.getDisconnectCause(), Matchers.isA(disconnectCauseClass));
+            assertThat(disconnectionHistory.getDisconnectCause(), Matchers.isA(disconnectCauseClass));
         } else {
-            assertNull(nodeTarget.disconnectionHistory.getDisconnectCause());
+            assertNull(disconnectionHistory.getDisconnectCause());
         }
     }