alex-plekhanov commented on code in PR #11637:
URL: https://github.com/apache/ignite/pull/11637#discussion_r1837033632
##########
modules/core/src/test/java/org/apache/ignite/spi/discovery/tcp/TcpDiscoverySelfTest.java:
##########
@@ -2201,6 +2211,27 @@ public void testDuplicatedDiscoveryDataRemoved() throws
Exception {
*/
@Test
public void testFailedNodeRestoreConnection() throws Exception {
+ assertFalse(checkRestoreConnection());
+
+ setLoggerDebugLevel();
+
+ assertTrue(checkRestoreConnection());
+ }
+
+ /** @throws Exception If failed.*/
+ private boolean checkRestoreConnection() throws Exception {
+ ListeningTestLogger testLog = new ListeningTestLogger(log);
Review Comment:
Local variable is redundant, let's use testLog field.
##########
modules/core/src/main/java/org/apache/ignite/spi/discovery/tcp/ServerImpl.java:
##########
@@ -982,6 +987,43 @@ else if (!spi.failureDetectionTimeoutEnabled() && reconCnt
== spi.getReconnectCo
}
}
+ /**
+ * Pings the node by its address to see if it's alive.
+ *
+ * @param addr Address of the node.
+ * @param nodeId Node ID to ping. In case when client node ID is not null
this node ID is an ID of the router node.
+ * @param clientNodeId Client node ID.
+ * @param timeout Timeout on operation in milliseconds. If 0, a value
based on {@link TcpDiscoverySpi} is used.
+ * @param debugLogging Boolean flag indicating whether debug information
should be printed into node log.
+ * @return ID of the remote node and "client exists" flag if node alive or
{@code null} if the remote node has
+ * left a topology during the ping process.
+ * @throws IgniteCheckedException If an error occurs.
+ * @see #pingNodeImpl(InetSocketAddress, UUID, UUID, long, boolean)
+ */
+ @Nullable private IgniteBiTuple<UUID, Boolean> pingNode(
+ InetSocketAddress addr,
+ @Nullable UUID nodeId,
+ @Nullable UUID clientNodeId,
+ long timeout,
+ boolean debugLogging) throws IgniteCheckedException {
+ try {
+ return pingNodeImpl(addr, nodeId, clientNodeId, timeout,
debugLogging);
+ }
+ catch (IgniteCheckedException e) {
+ logPingError(e.getMessage(), debugLogging);
+
+ throw e;
+ }
+ }
Review Comment:
Additional wrapper is redundant. Let's rename `pingNodeImpl` back to
`pingNode` and use it with one additional parameter
##########
modules/core/src/main/java/org/apache/ignite/spi/discovery/tcp/ServerImpl.java:
##########
@@ -819,15 +819,17 @@ private boolean pingNode(TcpDiscoveryNode node) {
* @param nodeId Node ID to ping. In case when client node ID is not null
this node ID is an ID of the router node.
* @param clientNodeId Client node ID.
* @param timeout Timeout on operation in milliseconds. If 0, a value
based on {@link TcpDiscoverySpi} is used.
+ * @param debugLogging Boolean flag indicating whether debug information
should be printed into node log.
Review Comment:
Javadoc is confusing
##########
modules/core/src/test/java/org/apache/ignite/spi/discovery/tcp/TcpDiscoverySelfTest.java:
##########
@@ -2251,6 +2286,8 @@ public void testCheckRingLatency() throws Exception {
testLog.registerListener(lsnr);
+ this.testLog = testLog;
+
try {
IgniteEx node =
startGrid(getConfiguration("server").setGridLogger(testLog));
Review Comment:
`setGridLogger(testLog)` is redundant now
##########
modules/core/src/test/java/org/apache/ignite/spi/discovery/tcp/TcpDiscoverySelfTest.java:
##########
@@ -2201,6 +2211,27 @@ public void testDuplicatedDiscoveryDataRemoved() throws
Exception {
*/
@Test
public void testFailedNodeRestoreConnection() throws Exception {
+ assertFalse(checkRestoreConnection());
+
+ setLoggerDebugLevel();
+
+ assertTrue(checkRestoreConnection());
+ }
+
+ /** @throws Exception If failed.*/
+ private boolean checkRestoreConnection() throws Exception {
+ ListeningTestLogger testLog = new ListeningTestLogger(log);
+
+ LogListener lsnr = LogListener
+ .matches(Pattern.compile("Failed to ping node \\[.*\\]\\.
Reached the timeout"))
+ .build();
+
+ testLog.registerListener(lsnr);
+
+ boolean result;
Review Comment:
Local variable is redundant. `return lsnr.check()` can be used. But it's
better to pass additional parameter for `checkRestoreConnection` and check it
at the end of the method, something like: `assertEquals(logExpected,
lsnr.check())`
##########
modules/core/src/test/java/org/apache/ignite/spi/discovery/tcp/TcpDiscoverySelfTest.java:
##########
@@ -2251,6 +2286,8 @@ public void testCheckRingLatency() throws Exception {
testLog.registerListener(lsnr);
+ this.testLog = testLog;
Review Comment:
Local variable is redundant now
##########
modules/core/src/main/java/org/apache/ignite/spi/discovery/tcp/ServerImpl.java:
##########
@@ -982,6 +987,43 @@ else if (!spi.failureDetectionTimeoutEnabled() && reconCnt
== spi.getReconnectCo
}
}
+ /**
+ * Pings the node by its address to see if it's alive.
+ *
+ * @param addr Address of the node.
+ * @param nodeId Node ID to ping. In case when client node ID is not null
this node ID is an ID of the router node.
+ * @param clientNodeId Client node ID.
+ * @param timeout Timeout on operation in milliseconds. If 0, a value
based on {@link TcpDiscoverySpi} is used.
+ * @param debugLogging Boolean flag indicating whether debug information
should be printed into node log.
+ * @return ID of the remote node and "client exists" flag if node alive or
{@code null} if the remote node has
+ * left a topology during the ping process.
+ * @throws IgniteCheckedException If an error occurs.
+ * @see #pingNodeImpl(InetSocketAddress, UUID, UUID, long, boolean)
+ */
+ @Nullable private IgniteBiTuple<UUID, Boolean> pingNode(
+ InetSocketAddress addr,
+ @Nullable UUID nodeId,
+ @Nullable UUID clientNodeId,
+ long timeout,
+ boolean debugLogging) throws IgniteCheckedException {
+ try {
+ return pingNodeImpl(addr, nodeId, clientNodeId, timeout,
debugLogging);
+ }
+ catch (IgniteCheckedException e) {
+ logPingError(e.getMessage(), debugLogging);
+
+ throw e;
+ }
+ }
+
+ /** */
+ private void logPingError(String msg, boolean debugLogging) {
+ if (!debugLogging)
Review Comment:
Let's invert logic and rename flag to `boolean logError`
--
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: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
