pivotal-jbarrett commented on code in PR #7381: URL: https://github.com/apache/geode/pull/7381#discussion_r860247826
########## geode-core/src/main/java/org/apache/geode/internal/tcp/TCPConduit.java: ########## @@ -909,6 +906,104 @@ public Connection getConnection(InternalDistributedMember memberAddress, } } + + /** + * Return a connection to the given member. This method performs quick scan for connection. + * Only one attempt to create a connection to the given member . + * + * @param memberAddress the IDS associated with the remoteId + * @param preserveOrder whether this is an ordered or unordered connection + * @param startTime the time this operation started + * @param ackTimeout the ack-wait-threshold * 1000 for the operation to be transmitted (or zero) + * @param ackSATimeout the ack-severe-alert-threshold * 1000 for the operation to be transmitted + * (or zero) + * + * @return the connection + */ + public Connection getFirstScanForConnection(InternalDistributedMember memberAddress, + final boolean preserveOrder, long startTime, long ackTimeout, + long ackSATimeout) throws IOException, DistributedSystemDisconnectedException { + if (stopped) { + throw new DistributedSystemDisconnectedException("The conduit is stopped"); + } + + Connection connection = null; + stopper.checkCancelInProgress(null); + boolean interrupted = Thread.interrupted(); + try { + // If this is the second time through this loop, we had problems. Review Comment: What loop? ########## geode-core/src/main/java/org/apache/geode/internal/tcp/TCPConduit.java: ########## @@ -909,6 +906,104 @@ public Connection getConnection(InternalDistributedMember memberAddress, } } + + /** + * Return a connection to the given member. This method performs quick scan for connection. + * Only one attempt to create a connection to the given member . + * + * @param memberAddress the IDS associated with the remoteId + * @param preserveOrder whether this is an ordered or unordered connection + * @param startTime the time this operation started + * @param ackTimeout the ack-wait-threshold * 1000 for the operation to be transmitted (or zero) + * @param ackSATimeout the ack-severe-alert-threshold * 1000 for the operation to be transmitted + * (or zero) + * + * @return the connection + */ + public Connection getFirstScanForConnection(InternalDistributedMember memberAddress, + final boolean preserveOrder, long startTime, long ackTimeout, + long ackSATimeout) throws IOException, DistributedSystemDisconnectedException { + if (stopped) { + throw new DistributedSystemDisconnectedException("The conduit is stopped"); + } + + Connection connection = null; + stopper.checkCancelInProgress(null); + boolean interrupted = Thread.interrupted(); + try { + // If this is the second time through this loop, we had problems. + // Tear down the connection so that it gets rebuilt. + + Exception problem = null; + try { + // Get (or regenerate) the connection + // this could generate a ConnectionException, so it must be caught and retried + boolean retryForOldConnection; + boolean debugRetry = false; + do { + retryForOldConnection = false; + connection = getConTable().get(memberAddress, preserveOrder, startTime, ackTimeout, + ackSATimeout, true); + if (connection == null) { + // conduit may be closed - otherwise an ioexception would be thrown + problem = new IOException( + String.format("Unable to reconnect to server; possible shutdown: %s", + memberAddress)); + } else if (connection.isClosing() + || !connection.getRemoteAddress().equals(memberAddress)) { + if (logger.isDebugEnabled()) { + logger.debug("Got an old connection for {}: {}@{}", memberAddress, connection, + connection.hashCode()); + } + connection.closeOldConnection("closing old connection"); + connection = null; + retryForOldConnection = true; Review Comment: I am rarely a fan of do/while loops and this one tracking this exit value was initially hard to follow. I think a infinite loop with throw exceptions and early breaks shortens it and clears up the purpose. ```java final debugEnabled = logger.isDebugEnabled(); while(true) { connection = getConTable()...; if (connection == null) { throw new IOException(...); } if (!(connection.isClosing() || ..)) { break; } if (debugEnabled) { logger.debug(...); } connection.closeOldConnection(...); } ``` Throwing the exception gets handled below the same as having set the `problem` variable. You don't need to reset the `connection` to `null` after it is closed since the next statement to execute sets it anyway. I would even go one step further and move this whole loop logic into a method. ```java final Connection = getConnectionThatIsNotClosed(...); ``` ```java @NotNull Connection getConnectionThatIsNotClosed(...) { final debugEnabled = logger.isDebugEnabled(); while(true) { final Connection connection = getConTable()...; if (connection == null) { throw new IOException(...); } if (!(connection.isClosing() || ..)) { return connection; } if (debugEnabled) { logger.debug(...); } connection.closeOldConnection(...); } } ``` ########## geode-core/src/main/java/org/apache/geode/internal/tcp/TCPConduit.java: ########## @@ -909,6 +906,104 @@ public Connection getConnection(InternalDistributedMember memberAddress, } } + + /** + * Return a connection to the given member. This method performs quick scan for connection. + * Only one attempt to create a connection to the given member . + * + * @param memberAddress the IDS associated with the remoteId + * @param preserveOrder whether this is an ordered or unordered connection + * @param startTime the time this operation started + * @param ackTimeout the ack-wait-threshold * 1000 for the operation to be transmitted (or zero) + * @param ackSATimeout the ack-severe-alert-threshold * 1000 for the operation to be transmitted + * (or zero) + * + * @return the connection + */ + public Connection getFirstScanForConnection(InternalDistributedMember memberAddress, + final boolean preserveOrder, long startTime, long ackTimeout, + long ackSATimeout) throws IOException, DistributedSystemDisconnectedException { + if (stopped) { + throw new DistributedSystemDisconnectedException("The conduit is stopped"); + } + + Connection connection = null; + stopper.checkCancelInProgress(null); + boolean interrupted = Thread.interrupted(); + try { + // If this is the second time through this loop, we had problems. + // Tear down the connection so that it gets rebuilt. + + Exception problem = null; + try { + // Get (or regenerate) the connection + // this could generate a ConnectionException, so it must be caught and retried + boolean retryForOldConnection; + boolean debugRetry = false; + do { + retryForOldConnection = false; + connection = getConTable().get(memberAddress, preserveOrder, startTime, ackTimeout, + ackSATimeout, true); + if (connection == null) { + // conduit may be closed - otherwise an ioexception would be thrown + problem = new IOException( + String.format("Unable to reconnect to server; possible shutdown: %s", + memberAddress)); + } else if (connection.isClosing() + || !connection.getRemoteAddress().equals(memberAddress)) { + if (logger.isDebugEnabled()) { + logger.debug("Got an old connection for {}: {}@{}", memberAddress, connection, + connection.hashCode()); + } + connection.closeOldConnection("closing old connection"); + connection = null; + retryForOldConnection = true; + debugRetry = true; + } + } while (retryForOldConnection); + if (debugRetry && logger.isDebugEnabled()) { Review Comment: Does this log message do anything for us? -- 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...@geode.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org