Use TimveValue instead of long for CacheBuilder methods

This changes the CacheBuilder methods that are used to set expiration times to accept a
TimeValue instead of long. Accepting a long can lead to issues where the incorrect value is
passed in as the time unit is not clearly identified. By using TimeValue the caller no longer
needs to worry about the time unit used by the cache or builder.

core/src/main/java/org/elasticsearch/common/cache/Cache.java

@@ -67,13 +67,13 @@ import java.util.function.ToLongBiFunction;
  */
 public class Cache<K, V> {
     // positive if entries have an expiration
-    private long expireAfterAccess = -1;
+    private long expireAfterAccessNanos = -1;
 
     // true if entries can expire after access
     private boolean entriesExpireAfterAccess;
 
     // positive if entries have an expiration after write
-    private long expireAfterWrite = -1;
+    private long expireAfterWriteNanos = -1;
 
     // true if entries can expire after initial insertion
     private boolean entriesExpireAfterWrite;
@@ -98,22 +98,32 @@ public class Cache<K, V> {
     Cache() {
     }
 
-    void setExpireAfterAccess(long expireAfterAccess) {
+    void setExpireAfterAccessNanos(long expireAfterAccessNanos) {
-        if (expireAfterAccess <= 0) {
+        if (expireAfterAccessNanos <= 0) {
-            throw new IllegalArgumentException("expireAfterAccess <= 0");
+            throw new IllegalArgumentException("expireAfterAccessNanos <= 0");
         }
-        this.expireAfterAccess = expireAfterAccess;
+        this.expireAfterAccessNanos = expireAfterAccessNanos;
         this.entriesExpireAfterAccess = true;
     }
 
-    void setExpireAfterWrite(long expireAfterWrite) {
+    // pkg-private for testing
-        if (expireAfterWrite <= 0) {
+    long getExpireAfterAccessNanos() {
-            throw new IllegalArgumentException("expireAfterWrite <= 0");
+        return this.expireAfterAccessNanos;
+    }
+
+    void setExpireAfterWriteNanos(long expireAfterWriteNanos) {
+        if (expireAfterWriteNanos <= 0) {
+            throw new IllegalArgumentException("expireAfterWriteNanos <= 0");
         }
-        this.expireAfterWrite = expireAfterWrite;
+        this.expireAfterWriteNanos = expireAfterWriteNanos;
         this.entriesExpireAfterWrite = true;
     }
 
+    // pkg-private for testing
+    long getExpireAfterWriteNanos() {
+        return this.expireAfterWriteNanos;
+    }
+
     void setMaximumWeight(long maximumWeight) {
         if (maximumWeight < 0) {
             throw new IllegalArgumentException("maximumWeight < 0");
@@ -696,8 +706,8 @@ public class Cache<K, V> {
     }
 
     private boolean isExpired(Entry<K, V> entry, long now) {
-        return (entriesExpireAfterAccess && now - entry.accessTime > expireAfterAccess) ||
+        return (entriesExpireAfterAccess && now - entry.accessTime > expireAfterAccessNanos) ||
-                (entriesExpireAfterWrite && now - entry.writeTime > expireAfterWrite);
+                (entriesExpireAfterWrite && now - entry.writeTime > expireAfterWriteNanos);
     }
 
     private boolean unlink(Entry<K, V> entry) {

core/src/main/java/org/elasticsearch/common/cache/CacheBuilder.java

@@ -19,13 +19,15 @@
 
 package org.elasticsearch.common.cache;
 
+import org.elasticsearch.common.unit.TimeValue;
+
 import java.util.Objects;
 import java.util.function.ToLongBiFunction;
 
 public class CacheBuilder<K, V> {
     private long maximumWeight = -1;
-    private long expireAfterAccess = -1;
+    private long expireAfterAccessNanos = -1;
-    private long expireAfterWrite = -1;
+    private long expireAfterWriteNanos = -1;
     private ToLongBiFunction<K, V> weigher;
     private RemovalListener<K, V> removalListener;
 
@@ -44,19 +46,35 @@ public class CacheBuilder<K, V> {
         return this;
     }
 
-    public CacheBuilder<K, V> setExpireAfterAccess(long expireAfterAccess) {
+    /**
-        if (expireAfterAccess <= 0) {
+     * Sets the amount of time before an entry in the cache expires after it was last accessed.
+     *
+     * @param expireAfterAccess The amount of time before an entry expires after it was last accessed. Must not be {@code null} and must
+     *                          be greater than 0.
+     */
+    public CacheBuilder<K, V> setExpireAfterAccess(TimeValue expireAfterAccess) {
+        Objects.requireNonNull(expireAfterAccess);
+        final long expireAfterAccessNanos = expireAfterAccess.getNanos();
+        if (expireAfterAccessNanos <= 0) {
             throw new IllegalArgumentException("expireAfterAccess <= 0");
         }
-        this.expireAfterAccess = expireAfterAccess;
+        this.expireAfterAccessNanos = expireAfterAccessNanos;
         return this;
     }
 
-    public CacheBuilder<K, V> setExpireAfterWrite(long expireAfterWrite) {
+    /**
-        if (expireAfterWrite <= 0) {
+     * Sets the amount of time before an entry in the cache expires after it was written.
+     *
+     * @param expireAfterWrite The amount of time before an entry expires after it was written. Must not be {@code null} and must be
+     *                         greater than 0.
+     */
+    public CacheBuilder<K, V> setExpireAfterWrite(TimeValue expireAfterWrite) {
+        Objects.requireNonNull(expireAfterWrite);
+        final long expireAfterWriteNanos = expireAfterWrite.getNanos();
+        if (expireAfterWriteNanos <= 0) {
             throw new IllegalArgumentException("expireAfterWrite <= 0");
         }
-        this.expireAfterWrite = expireAfterWrite;
+        this.expireAfterWriteNanos = expireAfterWriteNanos;
         return this;
     }
 
@@ -77,11 +95,11 @@ public class CacheBuilder<K, V> {
         if (maximumWeight != -1) {
             cache.setMaximumWeight(maximumWeight);
         }
-        if (expireAfterAccess != -1) {
+        if (expireAfterAccessNanos != -1) {
-            cache.setExpireAfterAccess(expireAfterAccess);
+            cache.setExpireAfterAccessNanos(expireAfterAccessNanos);
         }
-        if (expireAfterWrite != -1) {
+        if (expireAfterWriteNanos != -1) {
-            cache.setExpireAfterWrite(expireAfterWrite);
+            cache.setExpireAfterWriteNanos(expireAfterWriteNanos);
         }
         if (weigher != null) {
             cache.setWeigher(weigher);

core/src/main/java/org/elasticsearch/indices/IndicesRequestCache.java

@@ -47,7 +47,6 @@ import java.util.Collections;
 import java.util.Iterator;
 import java.util.Set;
 import java.util.concurrent.ConcurrentMap;
-import java.util.concurrent.TimeUnit;
 
 /**
  * The indices request cache allows to cache a shard level request stage responses, helping with improving
@@ -90,7 +89,7 @@ public final class IndicesRequestCache extends AbstractComponent implements Remo
         CacheBuilder<Key, Value> cacheBuilder = CacheBuilder.<Key, Value>builder()
             .setMaximumWeight(sizeInBytes).weigher((k, v) -> k.ramBytesUsed() + v.ramBytesUsed()).removalListener(this);
         if (expire != null) {
-            cacheBuilder.setExpireAfterAccess(TimeUnit.MILLISECONDS.toNanos(expire.millis()));
+            cacheBuilder.setExpireAfterAccess(expire);
         }
         cache = cacheBuilder.build();
     }

core/src/main/java/org/elasticsearch/script/ScriptService.java

@@ -136,7 +136,7 @@ public class ScriptService extends AbstractComponent implements Closeable, Clust
 
         TimeValue cacheExpire = SCRIPT_CACHE_EXPIRE_SETTING.get(settings);
         if (cacheExpire.getNanos() != 0) {
-            cacheBuilder.setExpireAfterAccess(cacheExpire.nanos());
+            cacheBuilder.setExpireAfterAccess(cacheExpire);
         }
 
         logger.debug("using script cache with max_size [{}], expire [{}]", cacheMaxSize, cacheExpire);

core/src/test/java/org/elasticsearch/common/cache/CacheBuilderTests.java

@@ -0,0 +1,50 @@
+/*
+ * Licensed to Elasticsearch under one or more contributor
+ * license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright
+ * ownership. Elasticsearch licenses this file to you under
+ * the Apache License, Version 2.0 (the "License"); you may
+ * not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.elasticsearch.common.cache;
+
+import org.elasticsearch.common.unit.TimeValue;
+import org.elasticsearch.test.ESTestCase;
+
+import static org.hamcrest.Matchers.containsString;
+
+public class CacheBuilderTests extends ESTestCase {
+
+    public void testSettingExpireAfterAccess() {
+        IllegalArgumentException iae =
+            expectThrows(IllegalArgumentException.class, () -> CacheBuilder.builder().setExpireAfterAccess(TimeValue.MINUS_ONE));
+        assertThat(iae.getMessage(), containsString("expireAfterAccess <="));
+        iae = expectThrows(IllegalArgumentException.class, () -> CacheBuilder.builder().setExpireAfterAccess(TimeValue.ZERO));
+        assertThat(iae.getMessage(), containsString("expireAfterAccess <="));
+        final TimeValue timeValue = TimeValue.parseTimeValue(randomPositiveTimeValue(), "");
+        Cache<Object, Object> cache = CacheBuilder.builder().setExpireAfterAccess(timeValue).build();
+        assertEquals(timeValue.getNanos(), cache.getExpireAfterAccessNanos());
+    }
+
+    public void testSettingExpireAfterWrite() {
+        IllegalArgumentException iae =
+            expectThrows(IllegalArgumentException.class, () -> CacheBuilder.builder().setExpireAfterWrite(TimeValue.MINUS_ONE));
+        assertThat(iae.getMessage(), containsString("expireAfterWrite <="));
+        iae = expectThrows(IllegalArgumentException.class, () -> CacheBuilder.builder().setExpireAfterWrite(TimeValue.ZERO));
+        assertThat(iae.getMessage(), containsString("expireAfterWrite <="));
+        final TimeValue timeValue = TimeValue.parseTimeValue(randomPositiveTimeValue(), "");
+        Cache<Object, Object> cache = CacheBuilder.builder().setExpireAfterWrite(timeValue).build();
+        assertEquals(timeValue.getNanos(), cache.getExpireAfterWriteNanos());
+    }
+}

core/src/test/java/org/elasticsearch/common/cache/CacheTests.java

@@ -228,7 +228,7 @@ public class CacheTests extends ESTestCase {
                 return now.get();
             }
         };
-        cache.setExpireAfterAccess(1);
+        cache.setExpireAfterAccessNanos(1);
         List<Integer> evictedKeys = new ArrayList<>();
         cache.setRemovalListener(notification -> {
             assertEquals(RemovalNotification.RemovalReason.EVICTED, notification.getRemovalReason());
@@ -265,7 +265,7 @@ public class CacheTests extends ESTestCase {
                 return now.get();
             }
         };
-        cache.setExpireAfterWrite(1);
+        cache.setExpireAfterWriteNanos(1);
         List<Integer> evictedKeys = new ArrayList<>();
         cache.setRemovalListener(notification -> {
             assertEquals(RemovalNotification.RemovalReason.EVICTED, notification.getRemovalReason());
@@ -307,7 +307,7 @@ public class CacheTests extends ESTestCase {
                 return now.get();
             }
         };
-        cache.setExpireAfterAccess(1);
+        cache.setExpireAfterAccessNanos(1);
         now.set(0);
         for (int i = 0; i < numberOfEntries; i++) {
             cache.put(i, Integer.toString(i));