Fix default max interval calculation in FailureDetector

Patch by Sam Tunnicliffe; reviewed by Maxim Muzafarov and Stefan Miklosovic
for CASSANDRA-21025

Author: beobal

CHANGES.txt

@@ -1,4 +1,5 @@
 5.0.7
+ * Correctly calculate default for FailureDetector max interval (CASSANDRA-21025)
  * Adding missing configs in system_views.settings to be backward compatible (CASSANDRA-20863)
  * Heap dump should not be generated on handled exceptions (CASSANDRA-20974)
 Merged from 4.1:

src/java/org/apache/cassandra/gms/FailureDetector.java

@@ -43,6 +43,7 @@
 import javax.management.openmbean.TabularDataSupport;
 import javax.management.openmbean.TabularType;
 
+import com.google.common.annotations.VisibleForTesting;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -416,6 +417,17 @@ public String toString()
         sb.append("-----------------------------------------------------------------------");
         return sb.toString();
     }
+
+    /**
+     * Only for testing. In production code, ArrivalWindow instances call getMaxInterval() during
+     * intitialization and use the value to set the private final field MAX_INTERVAL_IN_NANO
+     * @return the value that would be used for to populate any new ArrivalWindow instance
+     */
+    @VisibleForTesting
+    static long calculateMaxInterval()
+    {
+        return ArrivalWindow.getMaxInterval();
+    }
 }
 
 /*
@@ -485,9 +497,10 @@ class ArrivalWindow
         arrivalIntervals = new ArrayBackedBoundedStats(size);
     }
 
-    private static long getMaxInterval()
+    @VisibleForTesting
+    static long getMaxInterval()
     {
-        long newValue = FD_MAX_INTERVAL_MS.getLong(FailureDetector.INITIAL_VALUE_NANOS);
+        long newValue = FD_MAX_INTERVAL_MS.getLong(TimeUnit.NANOSECONDS.toMillis(FailureDetector.INITIAL_VALUE_NANOS));
         if (newValue != FailureDetector.INITIAL_VALUE_NANOS)
             logger.info("Overriding {} from {}ms to {}ms", FD_MAX_INTERVAL_MS.getKey(), FailureDetector.INITIAL_VALUE_NANOS, newValue);
         return TimeUnit.NANOSECONDS.convert(newValue, TimeUnit.MILLISECONDS);

test/unit/org/apache/cassandra/gms/FailureDetectorTest.java

@@ -23,11 +23,13 @@
 import java.util.Collections;
 import java.util.List;
 import java.util.UUID;
+import java.util.concurrent.TimeUnit;
 
 import org.junit.BeforeClass;
 import org.junit.Test;
 
 import org.apache.cassandra.Util;
+import org.apache.cassandra.config.CassandraRelevantProperties;
 import org.apache.cassandra.config.DatabaseDescriptor;
 import org.apache.cassandra.db.commitlog.CommitLog;
 import org.apache.cassandra.dht.IPartitioner;
@@ -38,6 +40,7 @@
 import org.apache.cassandra.service.StorageService;
 
 import static org.apache.cassandra.config.CassandraRelevantProperties.MAX_LOCAL_PAUSE_IN_MS;
+import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 
 public class FailureDetectorTest
@@ -87,4 +90,36 @@ public void testConvictAfterLeft() throws UnknownHostException
         FailureDetector.instance.interpret(leftHost);
         assertFalse("Left endpoint not convicted", FailureDetector.instance.isAlive(leftHost));
     }
+
+    @Test
+    public void testMaxIntervalCalculation()
+    {
+        // Default value for ArrivalWindow.MAX_INTERVAL_IN_NANO, which is supplied by
+        // ArrivalWindow::getMaxInterval should be 2000000000ns/2 seconds.
+        Long initialPropertyValue = CassandraRelevantProperties.FD_MAX_INTERVAL_MS.isPresent()
+                                    ? CassandraRelevantProperties.FD_MAX_INTERVAL_MS.getLong()
+                                    : null;
+        try
+        {
+            // verify that max interval isn't being set directly using system property
+            CassandraRelevantProperties.FD_MAX_INTERVAL_MS.reset();
+            assertFalse(CassandraRelevantProperties.FD_MAX_INTERVAL_MS.isPresent());
+            // in which case, max interval should default to INITIAL_VALUE_NANOS
+            assertEquals(FailureDetector.INITIAL_VALUE_NANOS, FailureDetector.calculateMaxInterval());
+
+            // max interval can be overridden, but it's value should be supplied in millis
+            long overrideMillis = TimeUnit.NANOSECONDS.toMillis(FailureDetector.INITIAL_VALUE_NANOS * 2);
+            CassandraRelevantProperties.FD_MAX_INTERVAL_MS.setLong(overrideMillis);
+            // max interval is a nanos value, so convert the override to get the expected value
+            long expectedNanos = TimeUnit.NANOSECONDS.convert(overrideMillis, TimeUnit.MILLISECONDS);
+            assertEquals(expectedNanos, FailureDetector.calculateMaxInterval());
+        }
+        finally
+        {
+            if (initialPropertyValue == null)
+                CassandraRelevantProperties.FD_MAX_INTERVAL_MS.reset();
+            else
+                CassandraRelevantProperties.FD_MAX_INTERVAL_MS.setLong(initialPropertyValue);
+        }
+    }
 }