From 46eafa57e199e72893e998a4c7619991df47ea2c Mon Sep 17 00:00:00 2001
From: Lucas Capistrant <capistrant@users.noreply.github.com>
Date: Mon, 20 Feb 2023 06:02:27 -0600
Subject: [PATCH] Improve client change counter management in HTTP Server View
 (#13010)

* Avoid calling resolveWaitingFutures if there are no changes made

* Avoid telling HTTP serveview client to reset counter when their counter is valid
---
 .../coordination/ChangeRequestHistory.java    | 19 +++++++++++++++++--
 .../ChangeRequestHistoryTest.java             |  6 ++++++
 2 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/server/src/main/java/org/apache/druid/server/coordination/ChangeRequestHistory.java b/server/src/main/java/org/apache/druid/server/coordination/ChangeRequestHistory.java
index a0c45b4a335..7a4017e333c 100644
--- a/server/src/main/java/org/apache/druid/server/coordination/ChangeRequestHistory.java
+++ b/server/src/main/java/org/apache/druid/server/coordination/ChangeRequestHistory.java
@@ -94,6 +94,11 @@ public class ChangeRequestHistory<T>
     if (singleThreadedExecutor.isShutdown()) {
       return;
     }
+    // We don't want to resolve our futures if there aren't actually any change requests being added!
+    if (requests.isEmpty()) {
+      return;
+    }
+
     for (T request : requests) {
       changes.add(new Holder<>(request, getLastCounter().inc()));
     }
@@ -162,10 +167,20 @@ public class ChangeRequestHistory<T>
   {
     Counter lastCounter = getLastCounter();
 
-    if (counter.counter >= lastCounter.counter) {
+    if (counter.counter == lastCounter.counter) {
+      // We don't want to trigger a counter reset if the client counter matches the last counter! Return an empty list
+      // of changes instead.
+      if (counter.matches(lastCounter)) {
+        return ChangeRequestsSnapshot.success(counter, new ArrayList<>());
+      } else {
+        return ChangeRequestsSnapshot.fail(
+            StringUtils.format("counter[%s] failed to match with [%s]", counter, lastCounter)
+        );
+      }
+    } else if (counter.counter > lastCounter.counter) {
       return ChangeRequestsSnapshot.fail(
           StringUtils.format(
-              "counter[%s] >= last counter[%s]",
+              "counter[%s] > last counter[%s]",
               counter,
               lastCounter
           )
diff --git a/server/src/test/java/org/apache/druid/server/coordination/ChangeRequestHistoryTest.java b/server/src/test/java/org/apache/druid/server/coordination/ChangeRequestHistoryTest.java
index ecba53a8a03..62dcad86afa 100644
--- a/server/src/test/java/org/apache/druid/server/coordination/ChangeRequestHistoryTest.java
+++ b/server/src/test/java/org/apache/druid/server/coordination/ChangeRequestHistoryTest.java
@@ -19,6 +19,7 @@
 
 package org.apache.druid.server.coordination;
 
+import com.google.common.collect.ImmutableList;
 import com.google.common.util.concurrent.FutureCallback;
 import com.google.common.util.concurrent.Futures;
 import com.google.common.util.concurrent.ListenableFuture;
@@ -166,6 +167,11 @@ public class ChangeRequestHistoryTest
 
     Assert.assertFalse(future.isDone());
 
+    // An empty list of changes should not trigger the future to return!
+    history.addChangeRequests(ImmutableList.of());
+
+    Assert.assertFalse(future.isDone());
+
     history.addChangeRequest(new SegmentChangeRequestNoop());
 
     ChangeRequestsSnapshot snapshot = future.get(1, TimeUnit.MINUTES);