You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by "dchenbecker (via GitHub)" <gi...@apache.org> on 2023/02/06 17:00:10 UTC
[GitHub] [cassandra] dchenbecker opened a new pull request, #2143: [CASSANDRA-17979] Remove invalid handshake test assertion
dchenbecker opened a new pull request, #2143:
URL: https://github.com/apache/cassandra/pull/2143
The call to hasPending is not dependent on the isConnected call, and the test is to ensure that handshake (e.g. connection) works, not that messages are flushed. If we want to make some assertions on message delivery, they should be in a different test and will need different logic.
Fixes CASSANDRA-17979
patch by Derek Chen-Becker; reviewed by TBD for CASSANDRA-17979
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] ekaterinadimitrova2 commented on a diff in pull request #2143: [CASSANDRA-17979] Remove invalid handshake test assertion
Posted by "ekaterinadimitrova2 (via GitHub)" <gi...@apache.org>.
ekaterinadimitrova2 commented on code in PR #2143:
URL: https://github.com/apache/cassandra/pull/2143#discussion_r1102022551
##########
test/unit/org/apache/cassandra/net/HandshakeTest.java:
##########
@@ -377,24 +378,35 @@ private void testOutboundFallbackOnSSLHandshakeFailure(SslFallbackConnectionType
InetAddressAndPort endpoint = inbound.sockets().stream().map(s -> s.settings.bindAddress).findFirst().get();
inbound.open();
- // Open outbound connections, and wait until connection is established
+ // Open outbound connections, and wait until connection is established and the initial message is delivered
OutboundConnection outboundConnection = initiateOutbound(endpoint, fromConnectionType, fromOptional);
- waitForConnection(outboundConnection);
- assertTrue(outboundConnection.isConnected());
- assertFalse(outboundConnection.hasPending());
+ confirmDelivery(outboundConnection);
}
finally
{
inbound.close().await(10L, TimeUnit.SECONDS);
}
}
- private void waitForConnection(OutboundConnection outboundConnection) throws InterruptedException
+ private final static Duration DELIVERY_WAIT_DURATION = Duration.ofSeconds(60);
+
+ private void confirmDelivery(OutboundConnection outboundConnection) throws InterruptedException
{
- long startTime = System.currentTimeMillis();
- while (!outboundConnection.isConnected() && System.currentTimeMillis() - startTime < 60000)
+ Instant endTime = Instant.now().plus(DELIVERY_WAIT_DURATION);
+
+ while (!outboundConnection.isConnected() && Instant.now().isBefore(endTime))
{
Thread.sleep(1000);
}
+
+ // Now that we're connected, we also want to ensure that the delivery thread succeeded
+ while (outboundConnection.hasPending() && Instant.now().isBefore(endTime)) {
Review Comment:
`We might be waiting indefinitely`
@jyothsnakonisa Can you elaborate a bit, please? I suspect I might be missing something
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] dchenbecker commented on pull request #2143: [CASSANDRA-17979] Remove invalid handshake test assertion
Posted by "dchenbecker (via GitHub)" <gi...@apache.org>.
dchenbecker commented on PR #2143:
URL: https://github.com/apache/cassandra/pull/2143#issuecomment-1440575485
This was merged separately, so I'm closing this PR
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] jyothsnakonisa commented on a diff in pull request #2143: [CASSANDRA-17979] Remove invalid handshake test assertion
Posted by "jyothsnakonisa (via GitHub)" <gi...@apache.org>.
jyothsnakonisa commented on code in PR #2143:
URL: https://github.com/apache/cassandra/pull/2143#discussion_r1101909473
##########
test/unit/org/apache/cassandra/net/HandshakeTest.java:
##########
@@ -377,24 +378,35 @@ private void testOutboundFallbackOnSSLHandshakeFailure(SslFallbackConnectionType
InetAddressAndPort endpoint = inbound.sockets().stream().map(s -> s.settings.bindAddress).findFirst().get();
inbound.open();
- // Open outbound connections, and wait until connection is established
+ // Open outbound connections, and wait until connection is established and the initial message is delivered
OutboundConnection outboundConnection = initiateOutbound(endpoint, fromConnectionType, fromOptional);
- waitForConnection(outboundConnection);
- assertTrue(outboundConnection.isConnected());
- assertFalse(outboundConnection.hasPending());
+ confirmDelivery(outboundConnection);
}
finally
{
inbound.close().await(10L, TimeUnit.SECONDS);
}
}
- private void waitForConnection(OutboundConnection outboundConnection) throws InterruptedException
+ private final static Duration DELIVERY_WAIT_DURATION = Duration.ofSeconds(60);
+
+ private void confirmDelivery(OutboundConnection outboundConnection) throws InterruptedException
{
- long startTime = System.currentTimeMillis();
- while (!outboundConnection.isConnected() && System.currentTimeMillis() - startTime < 60000)
+ Instant endTime = Instant.now().plus(DELIVERY_WAIT_DURATION);
+
+ while (!outboundConnection.isConnected() && Instant.now().isBefore(endTime))
{
Thread.sleep(1000);
}
+
+ // Now that we're connected, we also want to ensure that the delivery thread succeeded
+ while (outboundConnection.hasPending() && Instant.now().isBefore(endTime)) {
Review Comment:
I think we just want to verify that the connection is established and we don't have to wait for pending connections to be flushed. We might be waiting indefinitely for the pending connections to be flushed and the test might fail because of timeout.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] dchenbecker closed pull request #2143: [CASSANDRA-17979] Remove invalid handshake test assertion
Posted by "dchenbecker (via GitHub)" <gi...@apache.org>.
dchenbecker closed pull request #2143: [CASSANDRA-17979] Remove invalid handshake test assertion
URL: https://github.com/apache/cassandra/pull/2143
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] jyothsnakonisa commented on a diff in pull request #2143: [CASSANDRA-17979] Remove invalid handshake test assertion
Posted by "jyothsnakonisa (via GitHub)" <gi...@apache.org>.
jyothsnakonisa commented on code in PR #2143:
URL: https://github.com/apache/cassandra/pull/2143#discussion_r1101909473
##########
test/unit/org/apache/cassandra/net/HandshakeTest.java:
##########
@@ -377,24 +378,35 @@ private void testOutboundFallbackOnSSLHandshakeFailure(SslFallbackConnectionType
InetAddressAndPort endpoint = inbound.sockets().stream().map(s -> s.settings.bindAddress).findFirst().get();
inbound.open();
- // Open outbound connections, and wait until connection is established
+ // Open outbound connections, and wait until connection is established and the initial message is delivered
OutboundConnection outboundConnection = initiateOutbound(endpoint, fromConnectionType, fromOptional);
- waitForConnection(outboundConnection);
- assertTrue(outboundConnection.isConnected());
- assertFalse(outboundConnection.hasPending());
+ confirmDelivery(outboundConnection);
}
finally
{
inbound.close().await(10L, TimeUnit.SECONDS);
}
}
- private void waitForConnection(OutboundConnection outboundConnection) throws InterruptedException
+ private final static Duration DELIVERY_WAIT_DURATION = Duration.ofSeconds(60);
+
+ private void confirmDelivery(OutboundConnection outboundConnection) throws InterruptedException
{
- long startTime = System.currentTimeMillis();
- while (!outboundConnection.isConnected() && System.currentTimeMillis() - startTime < 60000)
+ Instant endTime = Instant.now().plus(DELIVERY_WAIT_DURATION);
+
+ while (!outboundConnection.isConnected() && Instant.now().isBefore(endTime))
{
Thread.sleep(1000);
}
+
+ // Now that we're connected, we also want to ensure that the delivery thread succeeded
+ while (outboundConnection.hasPending() && Instant.now().isBefore(endTime)) {
Review Comment:
I think we just want to verify that the connection is established and we don't have to wait for pending connections to be flushed. We might be waiting indefinitely for the pending connections to be flushed and the test might fail.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] dchenbecker commented on a diff in pull request #2143: [CASSANDRA-17979] Remove invalid handshake test assertion
Posted by "dchenbecker (via GitHub)" <gi...@apache.org>.
dchenbecker commented on code in PR #2143:
URL: https://github.com/apache/cassandra/pull/2143#discussion_r1102005705
##########
test/unit/org/apache/cassandra/net/HandshakeTest.java:
##########
@@ -377,24 +378,35 @@ private void testOutboundFallbackOnSSLHandshakeFailure(SslFallbackConnectionType
InetAddressAndPort endpoint = inbound.sockets().stream().map(s -> s.settings.bindAddress).findFirst().get();
inbound.open();
- // Open outbound connections, and wait until connection is established
+ // Open outbound connections, and wait until connection is established and the initial message is delivered
OutboundConnection outboundConnection = initiateOutbound(endpoint, fromConnectionType, fromOptional);
- waitForConnection(outboundConnection);
- assertTrue(outboundConnection.isConnected());
- assertFalse(outboundConnection.hasPending());
+ confirmDelivery(outboundConnection);
}
finally
{
inbound.close().await(10L, TimeUnit.SECONDS);
}
}
- private void waitForConnection(OutboundConnection outboundConnection) throws InterruptedException
+ private final static Duration DELIVERY_WAIT_DURATION = Duration.ofSeconds(60);
+
+ private void confirmDelivery(OutboundConnection outboundConnection) throws InterruptedException
{
- long startTime = System.currentTimeMillis();
- while (!outboundConnection.isConnected() && System.currentTimeMillis() - startTime < 60000)
+ Instant endTime = Instant.now().plus(DELIVERY_WAIT_DURATION);
+
+ while (!outboundConnection.isConnected() && Instant.now().isBefore(endTime))
{
Thread.sleep(1000);
}
+
+ // Now that we're connected, we also want to ensure that the delivery thread succeeded
+ while (outboundConnection.hasPending() && Instant.now().isBefore(endTime)) {
Review Comment:
I had made that argument in my original commit. @ekaterinadimitrova2 had pushed back and asked to also confirm delivery, so I made the change. Can you discuss in the thread and see what we can all agree on?
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org