Add the random delay before an available update as seconds

pipeline/pipeline.engines/src/main/java/fiftyone/pipeline/engines/services/DataUpdateServiceDefault.java

@@ -244,7 +244,7 @@ public void run() {
                                             checkForUpdate(dataFile, false);
                                         }
                                     },
-                                    getInterval(dataFile.getConfiguration())));
+                                    getIntervalMillis(dataFile.getConfiguration())));
                         }
                     }
                 }
@@ -435,19 +435,21 @@ public void registerDataFile(final AspectEngineDataFile dataFile) {
             synchronized (futureLock) {
                 if (aspectDataFile.getAutomaticUpdatesEnabled() &&
                         aspectDataFile.getFuture() == null) {
-                    long delay = getInterval(aspectDataFile.getConfiguration());
+                    long delayMillis =
+                            getIntervalMillis(aspectDataFile.getConfiguration());
                     if (aspectDataFile.getUpdateAvailableTime() != null) {
                         if (aspectDataFile.getUpdateAvailableTime().getTime()
                                 > System.currentTimeMillis()) {
-                            delay = applyIntervalRandomisation(
+                            delayMillis = applyIntervalRandomisationMillis(
                                     aspectDataFile.getUpdateAvailableTime().getTime()
                                             - System.currentTimeMillis(),
                                     aspectDataFile.getConfiguration());
                         }
                     }
                     // Create a timer that will go off when the engine expects
                     // updated data to be available.
-                    logger.debug("Scheduling update check with delay {}", delay);
+                    logger.debug("Scheduling update check with delay {} millis",
+                            delayMillis);
                     try {
                         ScheduledFuture<?> future = futureFactory.schedule(
                                 new Runnable() {
@@ -459,7 +461,7 @@ public void run() {
                                         logger.debug("Returning from checkForUpdate");
                                     }
                                 },
-                                delay);
+                                delayMillis);
                         logger.debug("Update check scheduled for {} millis",
                                 future.getDelay(TimeUnit.MILLISECONDS));
                         aspectDataFile.setFuture(future);
@@ -566,35 +568,37 @@ public void unregisterDataFile(AspectEngineDataFile dataFile) {
     }
 
     /**
-     * Get the random delay in milliseconds to add on to the scheduled time
-     * for a specific Engine, no greater than the Engine's max randomisation.
+     * Get the polling interval for a data file plus a random number of
+     * seconds, no greater than the configured maximum randomisation.
      *
-     * @param config to get the delay for
-     * @return a random delay in milliseconds.
+     * @param config to get the interval for
+     * @return the interval in milliseconds.
      */
-    private long getInterval(DataFileConfiguration config) {
-        int seconds = 0;
+    private long getIntervalMillis(DataFileConfiguration config) {
+        long intervalMillis = 0;
         if (config.getPollingIntervalSeconds() > 0) {
-            seconds = config.getPollingIntervalSeconds();
+            intervalMillis = config.getPollingIntervalSeconds() * 1000L;
         }
-        return applyIntervalRandomisation(seconds, config) * 1000;
-
+        return applyIntervalRandomisationMillis(intervalMillis, config);
     }
 
     /**
-     * Add a random amount of time to the specified interval
+     * Add a random number of whole seconds, up to the maximum in the
+     * configuration, to the specified interval.
      *
-     * @param interval the interval to add a random amount of time to
-     * @param config   the {@link DataFileConfiguration} object that specifies the
-     *                 maximum number of seconds to add
-     * @return the new interval
+     * @param intervalMillis the interval in milliseconds to add to
+     * @param config         the {@link DataFileConfiguration} object that
+     *                       specifies the maximum number of seconds to add
+     * @return the new interval in milliseconds
      */
-    private long applyIntervalRandomisation(long interval, DataFileConfiguration config) {
-        long seconds = 0;
+    private long applyIntervalRandomisationMillis(
+            long intervalMillis,
+            DataFileConfiguration config) {
+        long randomSeconds = 0;
         if (config.getMaxRandomisationSeconds() > 0) {
-            seconds = random.nextInt(config.getMaxRandomisationSeconds());
+            randomSeconds = random.nextInt(config.getMaxRandomisationSeconds());
         }
-        return interval + seconds;
+        return intervalMillis + randomSeconds * 1000;
     }
 
     /**

pipeline/pipeline.engines/src/test/java/fiftyone/pipeline/engines/services/DataUpdateServiceTests.java

@@ -42,6 +42,7 @@
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.Parameterized;
+import org.mockito.ArgumentCaptor;
 import org.mockito.ArgumentMatcher;
 import org.mockito.internal.matchers.LessOrEqual;
 import org.mockito.invocation.InvocationOnMock;
@@ -233,6 +234,46 @@ public boolean matches(Long aLong) {
         );
     }
 
+    @Test
+    public void DataUpdateService_Register_UpdateAvailableRandomDelayInSeconds() {
+        // Arrange
+        final int registrations = 50;
+        final long availableInMillis = 3600000;
+        final long maxDelayMillis = 600 * 1000;
+        final Date availableAt =
+            new Date(System.currentTimeMillis() + availableInMillis);
+        DataFileConfigurationDefault config = new DataFileConfigurationDefault();
+        config.setAutomaticUpdatesEnabled(true);
+        config.setMaxRandomisationSeconds(600);
+
+        // Act
+        for (int i = 0; i < registrations; i++) {
+            AspectEngineDataFileDefault file = new AspectEngineDataFileDefault();
+            file.setUpdateAvailableTime(availableAt);
+            file.setEngine(mock(OnPremiseAspectEngine.class));
+            file.setConfiguration(config);
+            dataUpdate.registerDataFile(file);
+        }
+
+        // Assert
+        // The delay is the time until the update is available plus a random
+        // number of seconds, so across many registrations at least one adds
+        // more than a second, and none adds more than the maximum.
+        ArgumentCaptor<Long> delays = ArgumentCaptor.forClass(Long.class);
+        verify(futureFactory, times(registrations)).schedule(
+            any(Runnable.class),
+            delays.capture());
+        long largestExtra = Long.MIN_VALUE;
+        for (long delay : delays.getAllValues()) {
+            long extra = delay - availableInMillis;
+            largestExtra = Math.max(largestExtra, extra);
+            assertTrue("Delay " + delay + " adds more than the maximum",
+                extra <= maxDelayMillis);
+        }
+        assertTrue("Largest random delay was " + largestExtra + " millis",
+            largestExtra > 1000);
+    }
+
     @Test
     public void DataUpdateService_Register_AutoUpdateConfiguredInterval() {
         // Arrange