Re: [PR] JAVA-3051: Memory leak [cassandra-java-driver]

["absurdfarce (via GitHub)" <gi...@apache.org>]

absurdfarce commented on code in PR #1743:
URL: https://github.com/apache/cassandra-java-driver/pull/1743#discussion_r1750812556


##########
core/src/main/java/com/datastax/oss/driver/internal/core/loadbalancing/DefaultLoadBalancingPolicy.java:
##########
@@ -274,40 +279,19 @@ protected boolean isBusy(@NonNull Node node, @NonNull Session session) {
   }
 
   protected boolean isResponseRateInsufficient(@NonNull Node node, long now) {
-    // response rate is considered insufficient when less than 2 responses were obtained in
-    // the past interval delimited by RESPONSE_COUNT_RESET_INTERVAL_NANOS.
-    if (responseTimes.containsKey(node)) {
-      AtomicLongArray array = responseTimes.get(node);
-      if (array.length() == 2) {
-        long threshold = now - RESPONSE_COUNT_RESET_INTERVAL_NANOS;
-        long leastRecent = array.get(0);
-        return leastRecent - threshold < 0;
-      }
-    }
-    return true;
+    NodeResponseRateSample sample = responseTimes.get(node);
+    return !(sample == null || sample.hasSufficientResponses(now));
   }
 
+  /**
+   * Synchronously updates the response times for the given node. It is synchronous because the
+   * {@link #DefaultLoadBalancingPolicy(com.datastax.oss.driver.api.core.context.DriverContext,
+   * java.lang.String) CacheLoader.load} assigned is synchronous.
+   *
+   * @param node The node to update.
+   */
   protected void updateResponseTimes(@NonNull Node node) {
-    responseTimes.compute(
-        node,
-        (n, array) -> {
-          // The array stores at most two timestamps, since we don't need more;
-          // the first one is always the least recent one, and hence the one to inspect.
-          long now = nanoTime();
-          if (array == null) {
-            array = new AtomicLongArray(1);
-            array.set(0, now);
-          } else if (array.length() == 1) {
-            long previous = array.get(0);
-            array = new AtomicLongArray(2);
-            array.set(0, previous);
-            array.set(1, now);
-          } else {
-            array.set(0, array.get(1));
-            array.set(1, now);
-          }
-          return array;
-        });
+    this.responseTimes.compute(node, (k, v) -> v == null ? new NodeResponseRateSample() : v.next());

Review Comment:
   My expectation is that it wouldn't matter since we're creating (and GCing) pretty small objects and the JVM usually handles that quite well but it's not unreasonable to confirm that.
   
   @SiyaoIsHiding is this something you can test?  I suppose if we can demonstrate roughly equivalent memory usage and throughput for a decent size load that would be fine.



-- 
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