java-driver icon indicating copy to clipboard operation
java-driver copied to clipboard

ClusterStressTest.clusters_should_not_leak_connections assume wrong number of connections

Open fruch opened this issue 1 year ago • 5 comments

since ccm change the default smp for scylla to be 2, there are extra connections open to a single node

and test fails like that:

expected [2] but found [3]

https://jenkins.scylladb.com/view/staging/job/scylla-staging/job/fruch/job/java-driver-matrix-test/74/testReport/3.11.2.4.com.datastax.driver.core/ClusterStressTest/clusters_should_not_leak_connections/

fruch avatar Jun 01 '23 15:06 fruch

So let's revert the offending commit?

mykaul avatar Jun 01 '23 15:06 mykaul

seem like there more test thats are affected by this default: NettyOptionsTest.should_invoke_netty_options_hooks_single_node

fruch avatar Jun 01 '23 15:06 fruch

So let's revert the offending commit?

For reference, this is the change: https://github.com/scylladb/scylla-ccm/pull/436

As I stated in that PR, change shouldn't be that complex (python-driver is working fine with already, also the cpp-driver)

so we just need to find those and mark them correctly (in the matrixes we'll handle old release), in this repo we should mark those tests it for next releases.

we do want most of the test working with more the one shared per node as much as possible, by default.

--- a/driver-core/src/test/java/com/datastax/driver/core/ClusterStressTest.java
+++ b/driver-core/src/test/java/com/datastax/driver/core/ClusterStressTest.java
@@ -35,6 +35,7 @@ import org.slf4j.LoggerFactory;
 import org.testng.annotations.AfterMethod;
 import org.testng.annotations.Test;

+@CCMConfig(jvmArgs = {"--smp", "1"})
 public class ClusterStressTest extends CCMTestsSupport {

   private static final Logger logger = LoggerFactory.getLogger(ClusterStressTest.class);
diff --git a/driver-core/src/test/java/com/datastax/driver/core/NettyOptionsTest.java b/driver-core/src/test/java/com/datastax/driver/core/NettyOptionsTest.java
index 5c7fc1fc4..cd47f02be 100644
--- a/driver-core/src/test/java/com/datastax/driver/core/NettyOptionsTest.java
+++ b/driver-core/src/test/java/com/datastax/driver/core/NettyOptionsTest.java
@@ -45,7 +45,9 @@ import org.mockito.stubbing.Answer;
 import org.testng.annotations.Test;

 @CreateCCM(PER_METHOD)
-@CCMConfig(createCluster = false)
+@CCMConfig(
+    createCluster = false,
+    jvmArgs = {"--smp", "1"})
 public class NettyOptionsTest extends CCMTestsSupport {

   @Test(groups = "short")

fruch avatar Jun 01 '23 16:06 fruch

I believe this is due to driver calculating connections per shard (while PoolingOptions limits are per host). If the limit is not evenly divisible by the number of shards, driver will end up being a little bit over the limit (with some edge cases, but there always will be at least limit number of connections).

The tests that currently fail due to that should be: PoolingOptionsIntegrationTest, NettyOptionsTest, SingleConnectionPoolTest (I think different number of connection messes up calculation of in flight queries in should_throttle_requests) and it seems ClusterStressTest too.

I've prepared workaround for 2 of those in #210. Said workaround duplicates the tests and makes 'Scylla variant' of them that calculates expected number of connections the way shard aware driver does. Ideal fix would be probably slightly extending PoolingOptions for shard awareness and following those options strictly but that may cause change in behaviour for current users and make merging upstream harder.

I plan to finish the workarounds next week due to other issues taking precedence right now.

Bouncheck avatar Jun 14 '23 08:06 Bouncheck

This should be passing now (or with the new version release), since #210 was merged. I've settled on the same workaround but without duplicating tests.

Bouncheck avatar Jun 30 '23 21:06 Bouncheck