From fdceb64072307c747b1c0b145775ef592cea42bd Mon Sep 17 00:00:00 2001 From: Jay Modi Date: Thu, 13 Oct 2016 09:27:31 -0400 Subject: [PATCH] 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. --- .../org/elasticsearch/common/cache/Cache.java | 34 ++++++++----- .../common/cache/CacheBuilder.java | 42 +++++++++++----- .../indices/IndicesRequestCache.java | 3 +- .../elasticsearch/script/ScriptService.java | 2 +- .../common/cache/CacheBuilderTests.java | 50 +++++++++++++++++++ .../common/cache/CacheTests.java | 6 +-- 6 files changed, 107 insertions(+), 30 deletions(-) create mode 100644 core/src/test/java/org/elasticsearch/common/cache/CacheBuilderTests.java diff --git a/core/src/main/java/org/elasticsearch/common/cache/Cache.java b/core/src/main/java/org/elasticsearch/common/cache/Cache.java index a42d01ccf72..cf8b58d0271 100644 --- a/core/src/main/java/org/elasticsearch/common/cache/Cache.java +++ b/core/src/main/java/org/elasticsearch/common/cache/Cache.java @@ -67,13 +67,13 @@ import java.util.function.ToLongBiFunction; */ public class Cache { // 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 { Cache() { } - void setExpireAfterAccess(long expireAfterAccess) { - if (expireAfterAccess <= 0) { - throw new IllegalArgumentException("expireAfterAccess <= 0"); + void setExpireAfterAccessNanos(long expireAfterAccessNanos) { + if (expireAfterAccessNanos <= 0) { + throw new IllegalArgumentException("expireAfterAccessNanos <= 0"); } - this.expireAfterAccess = expireAfterAccess; + this.expireAfterAccessNanos = expireAfterAccessNanos; this.entriesExpireAfterAccess = true; } - void setExpireAfterWrite(long expireAfterWrite) { - if (expireAfterWrite <= 0) { - throw new IllegalArgumentException("expireAfterWrite <= 0"); + // pkg-private for testing + long getExpireAfterAccessNanos() { + 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 { } private boolean isExpired(Entry entry, long now) { - return (entriesExpireAfterAccess && now - entry.accessTime > expireAfterAccess) || - (entriesExpireAfterWrite && now - entry.writeTime > expireAfterWrite); + return (entriesExpireAfterAccess && now - entry.accessTime > expireAfterAccessNanos) || + (entriesExpireAfterWrite && now - entry.writeTime > expireAfterWriteNanos); } private boolean unlink(Entry entry) { diff --git a/core/src/main/java/org/elasticsearch/common/cache/CacheBuilder.java b/core/src/main/java/org/elasticsearch/common/cache/CacheBuilder.java index ffb0e591180..67c8d508ba5 100644 --- a/core/src/main/java/org/elasticsearch/common/cache/CacheBuilder.java +++ b/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 { private long maximumWeight = -1; - private long expireAfterAccess = -1; - private long expireAfterWrite = -1; + private long expireAfterAccessNanos = -1; + private long expireAfterWriteNanos = -1; private ToLongBiFunction weigher; private RemovalListener removalListener; @@ -44,19 +46,35 @@ public class CacheBuilder { return this; } - public CacheBuilder 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 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 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 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 { if (maximumWeight != -1) { cache.setMaximumWeight(maximumWeight); } - if (expireAfterAccess != -1) { - cache.setExpireAfterAccess(expireAfterAccess); + if (expireAfterAccessNanos != -1) { + cache.setExpireAfterAccessNanos(expireAfterAccessNanos); } - if (expireAfterWrite != -1) { - cache.setExpireAfterWrite(expireAfterWrite); + if (expireAfterWriteNanos != -1) { + cache.setExpireAfterWriteNanos(expireAfterWriteNanos); } if (weigher != null) { cache.setWeigher(weigher); diff --git a/core/src/main/java/org/elasticsearch/indices/IndicesRequestCache.java b/core/src/main/java/org/elasticsearch/indices/IndicesRequestCache.java index ff3713a374f..a08f9ca1ad4 100644 --- a/core/src/main/java/org/elasticsearch/indices/IndicesRequestCache.java +++ b/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 cacheBuilder = CacheBuilder.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(); } diff --git a/core/src/main/java/org/elasticsearch/script/ScriptService.java b/core/src/main/java/org/elasticsearch/script/ScriptService.java index 24cb816fb15..2d6e07d12ee 100644 --- a/core/src/main/java/org/elasticsearch/script/ScriptService.java +++ b/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); diff --git a/core/src/test/java/org/elasticsearch/common/cache/CacheBuilderTests.java b/core/src/test/java/org/elasticsearch/common/cache/CacheBuilderTests.java new file mode 100644 index 00000000000..e0a5786e184 --- /dev/null +++ b/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 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 cache = CacheBuilder.builder().setExpireAfterWrite(timeValue).build(); + assertEquals(timeValue.getNanos(), cache.getExpireAfterWriteNanos()); + } +} diff --git a/core/src/test/java/org/elasticsearch/common/cache/CacheTests.java b/core/src/test/java/org/elasticsearch/common/cache/CacheTests.java index 3b88a3bdcfe..d8dbaa673a0 100644 --- a/core/src/test/java/org/elasticsearch/common/cache/CacheTests.java +++ b/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 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 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));