issue openlookeng#307

actual retry number not matching exchange.max-retry-count.
ahanapradhan · Mar 7, 2022 · 98e38f2 · 98e38f2
1 parent 460d15c
commit 98e38f2
Show file tree

Hide file tree

Showing 2 changed files with 18 additions and 10 deletions.
diff --git a/presto-main/src/main/java/io/prestosql/server/remotetask/Backoff.java b/presto-main/src/main/java/io/prestosql/server/remotetask/Backoff.java
@@ -136,10 +136,19 @@ public synchronized void success()
     {
         lastRequestStart = 0;
         firstFailureTime = 0;
-        failureCount = 0;
+        setFailureCount(0, false);
         lastFailureTime = 0;
     }
 
+    private synchronized void setFailureCount(int n, boolean isInc)
+    {
+        if (isInc) {
+            failureCount = failureCount + n;
+            return;
+        }
+        failureCount = n;
+    }
+
     /**
      * @return true if max retry failed, now it is time to check node status from HeartbeatFailureDetector
      */
@@ -148,7 +157,7 @@ public synchronized boolean maxTried()
         long now = ticker.read();
 
         lastFailureTime = now;
-        failureCount++;
+        setFailureCount(1, true);
         if (lastRequestStart != 0) {
             failureRequestTimeTotal += now - lastRequestStart;
             lastRequestStart = 0;
@@ -160,11 +169,10 @@ public synchronized boolean maxTried()
             return false;
         }
 
-        if (failureCount < minTries) {
+        if (getFailureCount() < minTries) {
             return false;
         }
-
-        return failureCount > maxTries;
+        return getFailureCount() >= maxTries;
     }
 
     /**
@@ -186,7 +194,7 @@ public synchronized boolean failure()
         long now = ticker.read();
 
         lastFailureTime = now;
-        failureCount++;
+        setFailureCount(1, true);
         if (lastRequestStart != 0) {
             failureRequestTimeTotal += now - lastRequestStart;
             lastRequestStart = 0;
@@ -198,7 +206,7 @@ public synchronized boolean failure()
             return false;
         }
 
-        if (failureCount < minTries) {
+        if (getFailureCount() < minTries) {
             return false;
         }
 
@@ -208,7 +216,7 @@ public synchronized boolean failure()
 
     public synchronized long getBackoffDelayNanos()
     {
-        int tmpFailureCount = (int) min(backoffDelayIntervalsNanos.length, this.failureCount);
+        int tmpFailureCount = (int) min(backoffDelayIntervalsNanos.length, getFailureCount());
         if (tmpFailureCount == 0) {
             return 0;
         }

diff --git a/presto-main/src/test/java/io/prestosql/operator/TestHttpPageBufferClient.java b/presto-main/src/test/java/io/prestosql/operator/TestHttpPageBufferClient.java
@@ -459,9 +459,9 @@ public void testMaxRetryFailedRemoteHostGone()
         assertEquals(callback.getPages().size(), 0);
         assertEquals(callback.getCompletedRequests(), 11);
         assertEquals(callback.getFinishedBuffers(), 0);
-        assertEquals(callback.getFailedBuffers(), 1);
+        assertEquals(callback.getFailedBuffers(), 2);
         assertInstanceOf(callback.getFailure(), PageTransportTimeoutException.class);
-        assertContains(callback.getFailure().getMessage(), WORKER_NODE_ERROR + " (http://localhost:8080/0 - 11 failures,");
+        assertContains(callback.getFailure().getMessage(), WORKER_NODE_ERROR + " (http://localhost:8080/0 - 10 failures,");
         assertStatus(client, location, "queued", 0, 11, 11, 11, "not scheduled");
     }