Modify the CleanerService to use a minimum

Users running the `CleanerService` should not be able to disable it (via a `-1` as the time setting) because they'll just shoot themselves in the foot. This PR changes the behavior to allow extensive amounts via the setting (e.g., they could set it to 2 years). By doing this via the `Setting`, we can avoid a lot of boilerplate code for verification as well. If we decide to allow it to be disabled, then the setting should be explicit. I've found that users tend to not understand setting times to `-1`.

With the internal `IndicesCleaner` runnable, I have also moved the rescheduling code to `onAfter` so that it always happens, even if the license makes it temporarily invalid.

I also think that we should allow the user to dynamically set the setting regardless of it being allowed -- and warn on it. This way they can set it when it's expired or during the trial, but it will take effect when they apply the paid license. I think that this will provide a better user experience so that they do not have to remember to re-set it later.

This also removes the `LocalExporter`-specific setting that allowed it to override the global retention. If we ever add another listener, then we should add exporter-specific settings to support this kind of functionality.

Adds some tests for the settings as well as for the service, while also removing now unneeded ones.

Original commit: elastic/x-pack-elasticsearch@3abd41807e

elasticsearch/x-pack/marvel/src/main/java/org/elasticsearch/marvel/MarvelSettings.java

@@ -32,6 +32,10 @@ public class MarvelSettings extends AbstractComponent {
     public static final String LEGACY_DATA_INDEX_NAME = ".marvel-es-data";
 
     public static final String HISTORY_DURATION_SETTING_NAME = "history.duration";
+    /**
+     * The minimum amount of time allowed for the history duration.
+     */
+    public static final TimeValue HISTORY_DURATION_MINIMUM = TimeValue.timeValueHours(24);
     public static final TimeValue MAX_LICENSE_GRACE_PERIOD = TimeValue.timeValueHours(7 * 24);
 
     /**
@@ -101,10 +105,21 @@ public class MarvelSettings extends AbstractComponent {
             listSetting(key("agent.collectors"), Collections.emptyList(), Function.identity(), Property.NodeScope);
 
     /**
-     * The default retention duration of the monitoring history data
+     * The default retention duration of the monitoring history data.
+     * <p>
+     * Expected values:
+     * <ul>
+     * <li>Default: 7 days</li>
+     * <li>Minimum: 1 day</li>
+     * </ul>
+     *
+     * @see #HISTORY_DURATION_MINIMUM
      */
     public static final Setting<TimeValue> HISTORY_DURATION =
-            timeSetting(key(HISTORY_DURATION_SETTING_NAME), TimeValue.timeValueHours(7 * 24), Property.Dynamic, Property.NodeScope);
+            timeSetting(key(HISTORY_DURATION_SETTING_NAME),
+                        TimeValue.timeValueHours(7 * 24), // default value (7 days)
+                        HISTORY_DURATION_MINIMUM,         // minimum value
+                        Property.Dynamic, Property.NodeScope);
 
     /**
      * The index setting that holds the template version
@@ -221,7 +236,15 @@ public class MarvelSettings extends AbstractComponent {
         this.indices = indices.toArray(new String[0]);
     }
 
-    private static String key(String key) {
+    /**
+     * Prefix the {@code key} with the Monitoring prefix.
+     * <p>
+     * The {@code key} is prefixed by {@link XPackPlugin#featureSettingPrefix(String)}, {@link Marvel#NAME}, and {@code "."}.
+     *
+     * @param key The key to prefix
+     * @return The key prefixed by the product prefixes.
+     */
+    static String key(String key) {
         return XPackPlugin.featureSettingPrefix(Marvel.NAME) + "." + key;
     }
 

elasticsearch/x-pack/marvel/src/main/java/org/elasticsearch/marvel/agent/exporter/local/LocalExporter.java

@@ -254,19 +254,6 @@ public class LocalExporter extends Exporter implements ClusterStateListener, Cle
         }
 
         if (clusterService.localNode().masterNode()) {
-
-            // Retention duration can be overridden at exporter level
-            TimeValue exporterRetention = config.settings().getAsTime(MarvelSettings.HISTORY_DURATION_SETTING_NAME, null);
-            if (exporterRetention != null) {
-                try {
-                    cleanerService.validateRetention(exporterRetention);
-                    retention = exporterRetention;
-                } catch (IllegalArgumentException e) {
-                    logger.warn("local exporter [{}] - unable to use custom history duration [{}]: {}", name(), exporterRetention,
-                            e.getMessage());
-                }
-            }
-
             // Reference date time will be compared to index.creation_date settings,
             // that's why it must be in UTC
             DateTime expiration = new DateTime(DateTimeZone.UTC).minus(retention.millis());

elasticsearch/x-pack/marvel/src/main/java/org/elasticsearch/marvel/cleaner/CleanerService.java

@@ -23,18 +23,17 @@ import java.util.concurrent.CopyOnWriteArrayList;
 import java.util.concurrent.ScheduledFuture;
 
 /**
- * CleanerService takes care of deleting old monitoring indices.
+ * {@code CleanerService} takes care of deleting old monitoring indices.
  */
 public class CleanerService extends AbstractLifecycleComponent<CleanerService> {
 
-
     private final MarvelLicensee licensee;
     private final ThreadPool threadPool;
     private final ExecutionScheduler executionScheduler;
     private final List<Listener> listeners = new CopyOnWriteArrayList<>();
 
     private volatile IndicesCleaner runnable;
-    private volatile TimeValue retention;
+    private volatile TimeValue globalRetention;
 
     CleanerService(Settings settings, ClusterSettings clusterSettings, MarvelLicensee licensee, ThreadPool threadPool,
                    ExecutionScheduler executionScheduler) {
@@ -42,7 +41,10 @@ public class CleanerService extends AbstractLifecycleComponent<CleanerService> {
         this.licensee = licensee;
         this.threadPool = threadPool;
         this.executionScheduler = executionScheduler;
-        clusterSettings.addSettingsUpdateConsumer(MarvelSettings.HISTORY_DURATION, this::setRetention, this::validateRetention);
+        this.globalRetention = MarvelSettings.HISTORY_DURATION.get(settings);
+
+        // the validation is performed by the setting's object itself
+        clusterSettings.addSettingsUpdateConsumer(MarvelSettings.HISTORY_DURATION, this::setGlobalRetention);
     }
 
     @Inject
@@ -77,32 +79,60 @@ public class CleanerService extends AbstractLifecycleComponent<CleanerService> {
         return ThreadPool.Names.GENERIC;
     }
 
-    TimeValue getRetention() {
+    /**
+     * Get the retention that can be used.
+     * <p>
+     * This will ignore the global retention if the license does not allow retention updates.
+     *
+     * @return Never {@code null}
+     * @see MarvelLicensee#allowUpdateRetention()
+     */
+    public TimeValue getRetention() {
+        TimeValue retention;
+
+        // we only care about their value if they are allowed to set it
+        if (licensee.allowUpdateRetention() && globalRetention != null) {
+            retention = globalRetention;
+        }
+        else {
+            retention = MarvelSettings.HISTORY_DURATION.getDefault(Settings.EMPTY);
+        }
+
         return retention;
     }
 
-    public void setRetention(TimeValue retention) {
-        validateRetention(retention);
-        this.retention = retention;
-    }
-
-    public void validateRetention(TimeValue retention) {
-        if (retention == null) {
-            throw new IllegalArgumentException("history duration setting cannot be null");
-        }
-        if ((retention.getMillis() <= 0) && (retention.getMillis() != -1)) {
-            throw new IllegalArgumentException("invalid history duration setting value");
-        }
-        if (!licensee.allowUpdateRetention()) {
-            throw new IllegalArgumentException("license does not allow the history duration setting to be updated to value ["
-                    + retention + "]");
-        }
+    /**
+     * Set the global retention. This is expected to be used by the cluster settings to dynamically control the global retention time.
+     * <p>
+     * Even if the current license prevents retention updates, it will accept the change so that they do not need to re-set it if they
+     * upgrade their license (they can always unset it).
+     *
+     * @param globalRetention The global retention to use dynamically.
+     */
+    public void setGlobalRetention(TimeValue globalRetention) {
+        // notify the user that their setting will be ignored until they get the right license
+        if (licensee.allowUpdateRetention() == false) {
+            logger.warn("[{}] setting will be ignored until an appropriate license is applied", MarvelSettings.HISTORY_DURATION.getKey());
+        }
+
+        this.globalRetention = globalRetention;
     }
 
+    /**
+     * Add a {@code listener} that is executed by the internal {@code IndicesCleaner} given the {@link #getRetention() retention} time.
+     *
+     * @param listener A listener used to control retention
+     */
     public void add(Listener listener) {
         listeners.add(listener);
     }
 
+    /**
+     * Remove a {@code listener}.
+     *
+     * @param listener A listener used to control retention
+     * @see #add(Listener)
+     */
     public void remove(Listener listener) {
         listeners.remove(listener);
     }
@@ -121,6 +151,10 @@ public class CleanerService extends AbstractLifecycleComponent<CleanerService> {
         void onCleanUpIndices(TimeValue retention);
     }
 
+    /**
+     * {@code IndicesCleaner} runs and reschedules itself in order to automatically clean (delete) indices that are outside of the
+     * {@link #getRetention() retention} period.
+     */
     class IndicesCleaner extends AbstractRunnable {
 
         private volatile ScheduledFuture<?> future;
@@ -131,37 +165,39 @@ public class CleanerService extends AbstractLifecycleComponent<CleanerService> {
                 logger.trace("cleaning service is stopping, exiting");
                 return;
             }
-            if (!licensee.cleaningEnabled()) {
+            if (licensee.cleaningEnabled() == false) {
                 logger.debug("cleaning service is disabled due to invalid license");
                 return;
             }
 
-            TimeValue globalRetention = retention;
-            if (globalRetention == null) {
+            // fetch the retention, which is depends on a bunch of rules
+            TimeValue retention = getRetention();
+
+            logger.trace("cleaning up indices with retention [{}]", retention);
+
+            // Note: listeners are free to override the retention
+            for (Listener listener : listeners) {
                 try {
-                    globalRetention = MarvelSettings.HISTORY_DURATION.get(settings);
-                    validateRetention(globalRetention);
-                } catch (IllegalArgumentException e) {
-                    globalRetention = MarvelSettings.HISTORY_DURATION.get(Settings.EMPTY);
+                    listener.onCleanUpIndices(retention);
+                } catch (Throwable t) {
+                    logger.error("listener failed to clean indices", t);
                 }
             }
 
-            DateTime start = new DateTime(ISOChronology.getInstance());
-            if (globalRetention.millis() > 0) {
-                logger.trace("cleaning up indices with retention [{}]", globalRetention);
+            logger.trace("done cleaning up indices");
+        }
 
-                for (Listener listener : listeners) {
-                    try {
-                        listener.onCleanUpIndices(globalRetention);
-                    } catch (Throwable t) {
-                        logger.error("listener failed to clean indices", t);
-                    }
-                }
-            }
-
-            if (!lifecycle.stoppedOrClosed()) {
+        /**
+         * Reschedule the cleaner if the service is not stopped.
+         */
+        @Override
+        public void onAfter() {
+            if (lifecycle.stoppedOrClosed() == false) {
+                DateTime start = new DateTime(ISOChronology.getInstance());
                 TimeValue delay = executionScheduler.nextExecutionDelay(start);
+
                 logger.debug("scheduling next execution in [{}] seconds", delay.seconds());
+
                 future = threadPool.schedule(delay, executorName(), this);
             }
         }
@@ -171,6 +207,13 @@ public class CleanerService extends AbstractLifecycleComponent<CleanerService> {
             logger.error("failed to clean indices", t);
         }
 
+        /**
+         * Cancel/stop the cleaning service.
+         * <p>
+         * This will kill any scheduled {@link #future} from running. It's possible that this will be executed concurrently with the
+         * {@link #onAfter() rescheduling code}, at which point it will be stopped during the next execution <em>if</em> the service is
+         * stopped.
+         */
         public void cancel() {
             FutureUtils.cancel(future);
         }
@@ -179,8 +222,7 @@ public class CleanerService extends AbstractLifecycleComponent<CleanerService> {
     interface ExecutionScheduler {
 
         /**
-         * Calculates the delay in millis between "now" and
-         * the next execution.
+         * Calculates the delay in millis between "now" and the next execution.
          *
          * @param now the current time
          * @return the delay in millis
@@ -189,7 +231,7 @@ public class CleanerService extends AbstractLifecycleComponent<CleanerService> {
     }
 
     /**
-     * Schedule task so that it will be executed everyday at 01:00 AM
+     * Schedule task so that it will be executed everyday at 01:00 AM on the next day.
      */
     static class DefaultExecutionScheduler implements ExecutionScheduler {
 

elasticsearch/x-pack/marvel/src/test/java/org/elasticsearch/marvel/MarvelSettingsTests.java

@@ -0,0 +1,49 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License;
+ * you may not use this file except in compliance with the Elastic License.
+ */
+package org.elasticsearch.marvel;
+
+import org.elasticsearch.common.settings.Settings;
+import org.elasticsearch.common.unit.TimeValue;
+import org.elasticsearch.test.ESTestCase;
+
+import org.junit.Rule;
+import org.junit.rules.ExpectedException;
+
+/**
+ * Tests {@link MarvelSettings}
+ */
+public class MarvelSettingsTests extends ESTestCase {
+    @Rule
+    public ExpectedException expectedException = ExpectedException.none();
+
+    public void testHistoryDurationDefaults7Days() {
+        TimeValue sevenDays = TimeValue.timeValueHours(7 * 24);
+
+        // 7 days
+        assertEquals(sevenDays, MarvelSettings.HISTORY_DURATION.get(Settings.EMPTY));
+        // Note: this verifies the semantics because this is taken for granted that it never returns null!
+        assertEquals(sevenDays, MarvelSettings.HISTORY_DURATION.get(buildSettings(MarvelSettings.HISTORY_DURATION.getKey(), null)));
+    }
+
+    public void testHistoryDurationMinimum24Hours() {
+        // hit the minimum
+        assertEquals(MarvelSettings.HISTORY_DURATION_MINIMUM,
+                     MarvelSettings.HISTORY_DURATION.get(buildSettings(MarvelSettings.HISTORY_DURATION.getKey(), "24h")));
+    }
+
+    public void testHistoryDurationMinimum24HoursBlocksLower() {
+        expectedException.expect(IllegalArgumentException.class);
+
+        // 1 ms early!
+        String oneSecondEarly = (MarvelSettings.HISTORY_DURATION_MINIMUM.millis() - 1) + "ms";
+
+        MarvelSettings.HISTORY_DURATION.get(buildSettings(MarvelSettings.HISTORY_DURATION.getKey(), oneSecondEarly));
+    }
+
+    private Settings buildSettings(String key, String value) {
+        return Settings.builder().put(key, value).build();
+    }
+}

elasticsearch/x-pack/marvel/src/test/java/org/elasticsearch/marvel/cleaner/AbstractIndicesCleanerTestCase.java

@@ -7,9 +7,6 @@ package org.elasticsearch.marvel.cleaner;
 
 import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.common.unit.TimeValue;
-import org.elasticsearch.license.core.License;
-import org.elasticsearch.license.plugin.core.LicenseState;
-import org.elasticsearch.license.plugin.core.Licensee;
 import org.elasticsearch.marvel.MarvelSettings;
 import org.elasticsearch.marvel.MonitoredSystem;
 import org.elasticsearch.marvel.agent.exporter.Exporter;
@@ -17,7 +14,6 @@ import org.elasticsearch.marvel.agent.exporter.Exporters;
 import org.elasticsearch.marvel.agent.exporter.MarvelTemplateUtils;
 import org.elasticsearch.marvel.agent.exporter.MonitoringDoc;
 import org.elasticsearch.marvel.agent.resolver.MonitoringIndexNameResolver;
-import org.elasticsearch.marvel.license.MarvelLicensee;
 import org.elasticsearch.marvel.test.MarvelIntegTestCase;
 import org.elasticsearch.test.ESIntegTestCase.ClusterScope;
 import org.elasticsearch.test.VersionUtils;
@@ -27,8 +23,6 @@ import org.joda.time.DateTimeZone;
 import java.util.Locale;
 
 import static org.elasticsearch.test.ESIntegTestCase.Scope.TEST;
-import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked;
-import static org.hamcrest.Matchers.lessThanOrEqualTo;
 
 @ClusterScope(scope = TEST, numDataNodes = 0, numClientNodes = 0, transportClientRatio = 0.0)
 public abstract class AbstractIndicesCleanerTestCase extends MarvelIntegTestCase {
@@ -37,8 +31,7 @@ public abstract class AbstractIndicesCleanerTestCase extends MarvelIntegTestCase
     protected Settings nodeSettings(int nodeOrdinal) {
         Settings.Builder settings = Settings.builder()
                 .put(super.nodeSettings(nodeOrdinal))
-                .put(MarvelSettings.INTERVAL.getKey(), "-1")
-                .put(MarvelSettings.HISTORY_DURATION.getKey(), "-1");
+                .put(MarvelSettings.INTERVAL.getKey(), "-1");
         return settings.build();
     }
 
@@ -165,46 +158,6 @@ public abstract class AbstractIndicesCleanerTestCase extends MarvelIntegTestCase
         assertIndicesCount(retention);
     }
 
-    public void testRetentionAsExporterSetting() throws Exception {
-        final int max = 10;
-
-        // Default retention is between 3 and max days
-        final int defaultRetention = randomIntBetween(3, max);
-        internalCluster().startNode(Settings.builder().put(MarvelSettings.HISTORY_DURATION.getKey(),
-                String.format(Locale.ROOT, "%dd", defaultRetention)));
-
-        final DateTime now = now();
-        for (int i = 0; i < max; i++) {
-            createTimestampedIndex(MarvelTemplateUtils.TEMPLATE_VERSION, now.minusDays(i));
-        }
-        assertIndicesCount(max);
-
-        // Exporter retention is between 0 and the default retention
-        final int exporterRetention = randomIntBetween(1, defaultRetention);
-        assertThat(exporterRetention, lessThanOrEqualTo(defaultRetention));
-
-        // Updates the retention setting for the exporter
-        Exporters exporters = internalCluster().getInstance(Exporters.class);
-        for (Exporter exporter : exporters) {
-            Settings transientSettings = Settings.builder().put("xpack.monitoring.agent.exporters." + exporter.name() + "." +
-                    MarvelSettings.HISTORY_DURATION_SETTING_NAME, String.format(Locale.ROOT, "%dd", exporterRetention)).build();
-            assertAcked(client().admin().cluster().prepareUpdateSettings().setTransientSettings(transientSettings));
-        }
-
-        // Move to GOLD license
-        for (MarvelLicensee licensee : internalCluster().getInstances(MarvelLicensee.class)) {
-            licensee.onChange(new Licensee.Status(License.OperationMode.GOLD, LicenseState.ENABLED));
-        }
-
-        // Try to clean indices using the global setting
-        CleanerService.Listener listener = getListener();
-        listener.onCleanUpIndices(days(defaultRetention));
-
-        // Checks that indices have been deleted according to
-        // the retention configured at exporter level
-        assertIndicesCount(exporterRetention);
-    }
-
     protected CleanerService.Listener getListener() {
         Exporters exporters = internalCluster().getInstance(Exporters.class);
         for (Exporter exporter : exporters) {

elasticsearch/x-pack/marvel/src/test/java/org/elasticsearch/marvel/cleaner/CleanerServiceTests.java

@@ -16,26 +16,30 @@ import org.joda.time.DateTime;
 import org.joda.time.DateTimeZone;
 import org.junit.After;
 import org.junit.Before;
+import org.junit.Rule;
+import org.junit.rules.ExpectedException;
 
 import java.util.Collections;
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.TimeUnit;
 
-import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.equalTo;
 import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 
 public class CleanerServiceTests extends ESTestCase {
+    @Rule
+    public ExpectedException expectedException = ExpectedException.none();
 
+    private final MarvelLicensee licensee = mock(MarvelLicensee.class);
     private ClusterSettings clusterSettings;
-    private TimeValue defaultRetention;
     private ThreadPool threadPool;
 
     @Before
     public void start() {
         clusterSettings = new ClusterSettings(Settings.EMPTY, Collections.singleton(MarvelSettings.HISTORY_DURATION));
-        defaultRetention = TimeValue.parseTimeValue("7d", null, "");
         threadPool = new ThreadPool("CleanerServiceTests");
     }
 
@@ -44,64 +48,81 @@ public class CleanerServiceTests extends ESTestCase {
         terminate(threadPool);
     }
 
-    public void testRetentionDefaultValue() {
-        MarvelLicensee licensee = mock(MarvelLicensee.class);
-        when(licensee.allowUpdateRetention()).thenReturn(false);
-        assertNull(new CleanerService(Settings.EMPTY, clusterSettings, threadPool, licensee).getRetention());
+    public void testConstructorWithInvalidRetention() {
+        // invalid setting
+        expectedException.expect(IllegalArgumentException.class);
+
+        TimeValue expected = TimeValue.timeValueHours(1);
+        Settings settings = Settings.builder().put(MarvelSettings.HISTORY_DURATION.getKey(), expected.getStringRep()).build();
+
+        new CleanerService(settings, clusterSettings, threadPool, licensee);
     }
 
-    public void testRetentionUpdateAllowed() {
-        MarvelLicensee licensee = mock(MarvelLicensee.class);
+    public void testGetRetentionWithSettingWithUpdatesAllowed() {
+        TimeValue expected = TimeValue.timeValueHours(25);
+        Settings settings = Settings.builder().put(MarvelSettings.HISTORY_DURATION.getKey(), expected.getStringRep()).build();
+
+        when(licensee.allowUpdateRetention()).thenReturn(true);
+
+        assertEquals(expected, new CleanerService(settings, clusterSettings, threadPool, licensee).getRetention());
+
+        verify(licensee).allowUpdateRetention();
+    }
+
+    public void testGetRetentionDefaultValueWithNoSettings() {
+        when(licensee.allowUpdateRetention()).thenReturn(true);
+
+        assertEquals(MarvelSettings.HISTORY_DURATION.get(Settings.EMPTY),
+                     new CleanerService(Settings.EMPTY, clusterSettings, threadPool, licensee).getRetention());
+
+        verify(licensee).allowUpdateRetention();
+    }
+
+    public void testGetRetentionDefaultValueWithSettingsButUpdatesNotAllowed() {
+        TimeValue notExpected = TimeValue.timeValueHours(25);
+        Settings settings = Settings.builder().put(MarvelSettings.HISTORY_DURATION.getKey(), notExpected.getStringRep()).build();
+
+        when(licensee.allowUpdateRetention()).thenReturn(false);
+
+        assertEquals(MarvelSettings.HISTORY_DURATION.get(Settings.EMPTY),
+                     new CleanerService(settings, clusterSettings, threadPool, licensee).getRetention());
+
+        verify(licensee).allowUpdateRetention();
+    }
+
+    public void testSetGlobalRetention() {
+        // Note: I used this value to ensure we're not double-validating the setter; the cluster state should be the
+        // only thing calling this method and it will use the settings object to validate the time value
+        TimeValue expected = TimeValue.timeValueHours(2);
+
         when(licensee.allowUpdateRetention()).thenReturn(true);
 
         CleanerService service = new CleanerService(Settings.EMPTY, clusterSettings, threadPool, licensee);
-        service.setRetention(TimeValue.parseTimeValue("-1", null, ""));
-        assertThat(service.getRetention().getMillis(), equalTo(-1L));
 
-        TimeValue randomRetention = TimeValue.parseTimeValue(randomIntBetween(1, 1000) + "ms", null, "");
-        service.setRetention(randomRetention);
-        assertThat(service.getRetention(), equalTo(randomRetention));
+        service.setGlobalRetention(expected);
 
-        try {
-            service.validateRetention(randomRetention);
-        } catch (IllegalArgumentException e) {
-            fail("fail to validate new value of retention");
-        }
+        assertEquals(expected, service.getRetention());
+
+        verify(licensee, times(2)).allowUpdateRetention(); // once by set, once by get
     }
 
-    public void testRetentionUpdateBlocked() {
-        MarvelLicensee licensee = mock(MarvelLicensee.class);
-        when(licensee.allowUpdateRetention()).thenReturn(true);
+    public void testSetGlobalRetentionAppliesEvenIfLicenseDisallows() {
+        // Note: I used this value to ensure we're not double-validating the setter; the cluster state should be the
+        // only thing calling this method and it will use the settings object to validate the time value
+        TimeValue expected = TimeValue.timeValueHours(2);
+
+        // required to be true on the second call for it to see it take effect
+        when(licensee.allowUpdateRetention()).thenReturn(false).thenReturn(true);
+
         CleanerService service = new CleanerService(Settings.EMPTY, clusterSettings, threadPool, licensee);
-        try {
-            service.setRetention(TimeValue.parseTimeValue("-5000ms", null, ""));
-            fail("exception should have been thrown: negative retention are not allowed");
-        } catch (IllegalArgumentException e) {
-            assertThat(e.getMessage(), containsString("invalid history duration setting value"));
-        }
-        try {
-            service.setRetention(null);
-            fail("exception should have been thrown: null retention is not allowed");
-        } catch (IllegalArgumentException e) {
-            assertThat(e.getMessage(), containsString("history duration setting cannot be null"));
-        }
 
-        TimeValue randomRetention = TimeValue.parseTimeValue(randomIntBetween(1, 1000) + "ms", null, "");
-        when(licensee.allowUpdateRetention()).thenReturn(false);
-        try {
-            service.setRetention(randomRetention);
-            fail("exception should have been thrown");
-        } catch (IllegalArgumentException e) {
-            assertThat(e.getMessage(), containsString("license does not allow the history duration setting to be updated to value"));
-            assertNull(service.getRetention());
-        }
+        // uses allow=false
+        service.setGlobalRetention(expected);
 
-        try {
-            service.validateRetention(randomRetention);
-            fail("exception should have been thrown");
-        } catch (IllegalArgumentException e) {
-            assertThat(e.getMessage(), containsString("license does not allow the history duration setting to be updated to value"));
-        }
+        // uses allow=true
+        assertEquals(expected, service.getRetention());
+
+        verify(licensee, times(2)).allowUpdateRetention();
     }
 
     public void testNextExecutionDelay() {