From db004d0ac003724e55d099f3dfbea6fd3f9fee6d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Grzegorz=20Kokosi=C5=84ski?= Date: Tue, 25 Oct 2022 15:12:25 +0200 Subject: [PATCH 1/3] Expose NonKeyEvictableLoadingCache::unsafeInvalidate That method allows invalidation for a key where it is known that there are no ongoing loads for it. --- .../src/main/java/io/trino/execution/SqlTaskManager.java | 2 +- .../trino/collect/cache/NonKeyEvictableLoadingCache.java | 9 +++++++++ .../collect/cache/NonKeyEvictableLoadingCacheImpl.java | 6 ++++++ 3 files changed, 16 insertions(+), 1 deletion(-) diff --git a/core/trino-main/src/main/java/io/trino/execution/SqlTaskManager.java b/core/trino-main/src/main/java/io/trino/execution/SqlTaskManager.java index f6f7a3c2e89d..1791792dfca6 100644 --- a/core/trino-main/src/main/java/io/trino/execution/SqlTaskManager.java +++ b/core/trino-main/src/main/java/io/trino/execution/SqlTaskManager.java @@ -592,7 +592,7 @@ void removeOldTasks() if (endTime != null && endTime.isBefore(oldestAllowedTask)) { // The removal here is concurrency safe with respect to any concurrent loads: the cache has no expiration, // the taskId is in the cache, so there mustn't be an ongoing load. - tasks.asMap().remove(taskId); + tasks.unsafeInvalidate(taskId); } } catch (RuntimeException e) { diff --git a/lib/trino-collect/src/main/java/io/trino/collect/cache/NonKeyEvictableLoadingCache.java b/lib/trino-collect/src/main/java/io/trino/collect/cache/NonKeyEvictableLoadingCache.java index 84b04f95b45d..99f2ddc92f30 100644 --- a/lib/trino-collect/src/main/java/io/trino/collect/cache/NonKeyEvictableLoadingCache.java +++ b/lib/trino-collect/src/main/java/io/trino/collect/cache/NonKeyEvictableLoadingCache.java @@ -28,6 +28,15 @@ public interface NonKeyEvictableLoadingCache @Override void invalidate(Object key); + /** + * Allows invalidation for a key, but does not invalidate ongoing loads. + * It is caller responsibility to ensure correct use. + * The method is deprecated to discourage the usage of it. + * Use {@link EvictableCacheBuilder} to build a cache instead. + */ + @Deprecated + void unsafeInvalidate(Object key); + /** * @deprecated Not supported. Use {@link EvictableCacheBuilder} to build a cache instead. */ diff --git a/lib/trino-collect/src/main/java/io/trino/collect/cache/NonKeyEvictableLoadingCacheImpl.java b/lib/trino-collect/src/main/java/io/trino/collect/cache/NonKeyEvictableLoadingCacheImpl.java index e0814d33f213..34ae40a1da0c 100644 --- a/lib/trino-collect/src/main/java/io/trino/collect/cache/NonKeyEvictableLoadingCacheImpl.java +++ b/lib/trino-collect/src/main/java/io/trino/collect/cache/NonKeyEvictableLoadingCacheImpl.java @@ -43,6 +43,12 @@ public void invalidate(Object key) "Use EvictableCacheBuilder if you need invalidation"); } + @Override + public void unsafeInvalidate(Object key) + { + super.invalidate(key); + } + @Override public void invalidateAll(Iterable keys) { From 2af32a0d6250926cace4eec81180490a7760c62e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Grzegorz=20Kokosi=C5=84ski?= Date: Tue, 25 Oct 2022 15:12:26 +0200 Subject: [PATCH 2/3] Add test for SafeCaches Also makes error messages to be consistent --- .../cache/NonEvictableLoadingCacheImpl.java | 2 +- .../NonKeyEvictableLoadingCacheImpl.java | 4 +- .../trino/collect/cache/TestSafeCaches.java | 114 ++++++++++++++++++ 3 files changed, 117 insertions(+), 3 deletions(-) create mode 100644 lib/trino-collect/src/test/java/io/trino/collect/cache/TestSafeCaches.java diff --git a/lib/trino-collect/src/main/java/io/trino/collect/cache/NonEvictableLoadingCacheImpl.java b/lib/trino-collect/src/main/java/io/trino/collect/cache/NonEvictableLoadingCacheImpl.java index 2923e26f7568..dcf16f0cac1d 100644 --- a/lib/trino-collect/src/main/java/io/trino/collect/cache/NonEvictableLoadingCacheImpl.java +++ b/lib/trino-collect/src/main/java/io/trino/collect/cache/NonEvictableLoadingCacheImpl.java @@ -29,7 +29,7 @@ final class NonEvictableLoadingCacheImpl public void invalidateAll() { throw new UnsupportedOperationException("invalidateAll does not invalidate ongoing loads, so a stale value may remain in the cache for ever. " + - "Use EvictableCacheBuilder if you need invalidation, or use SafeCaches.buildNonEvictableCacheWithWeakInvalidateAll() " + + "Use EvictableCache if you need invalidation, or use SafeCaches.buildNonEvictableCacheWithWeakInvalidateAll() " + "if invalidateAll is not required for correctness"); } } diff --git a/lib/trino-collect/src/main/java/io/trino/collect/cache/NonKeyEvictableLoadingCacheImpl.java b/lib/trino-collect/src/main/java/io/trino/collect/cache/NonKeyEvictableLoadingCacheImpl.java index 34ae40a1da0c..235e4a696b78 100644 --- a/lib/trino-collect/src/main/java/io/trino/collect/cache/NonKeyEvictableLoadingCacheImpl.java +++ b/lib/trino-collect/src/main/java/io/trino/collect/cache/NonKeyEvictableLoadingCacheImpl.java @@ -40,7 +40,7 @@ protected LoadingCache delegate() public void invalidate(Object key) { throw new UnsupportedOperationException("invalidate(key) does not invalidate ongoing loads, so a stale value may remain in the cache for ever. " + - "Use EvictableCacheBuilder if you need invalidation"); + "Use EvictableCache if you need invalidation"); } @Override @@ -53,6 +53,6 @@ public void unsafeInvalidate(Object key) public void invalidateAll(Iterable keys) { throw new UnsupportedOperationException("invalidateAll(keys) does not invalidate ongoing loads, so a stale value may remain in the cache for ever. " + - "Use EvictableCacheBuilder if you need invalidation"); + "Use EvictableCache if you need invalidation"); } } diff --git a/lib/trino-collect/src/test/java/io/trino/collect/cache/TestSafeCaches.java b/lib/trino-collect/src/test/java/io/trino/collect/cache/TestSafeCaches.java new file mode 100644 index 000000000000..b7b527f33871 --- /dev/null +++ b/lib/trino-collect/src/test/java/io/trino/collect/cache/TestSafeCaches.java @@ -0,0 +1,114 @@ +/* + * Licensed 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 io.trino.collect.cache; + +import com.google.common.cache.Cache; +import com.google.common.cache.CacheBuilder; +import com.google.common.cache.CacheLoader; +import com.google.common.collect.ImmutableList; +import org.testng.annotations.Test; + +import static io.trino.collect.cache.SafeCaches.buildNonEvictableCache; +import static io.trino.collect.cache.SafeCaches.buildNonEvictableCacheWithWeakInvalidateAll; +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.testng.Assert.assertSame; + +public class TestSafeCaches +{ + @Test + public void testNonEvictableCache() + throws Exception + { + NonEvictableCache cache = buildNonEvictableCache(CacheBuilder.newBuilder()); + verifyKeyInvalidationIsImpossible(cache); + verifyClearIsImpossible(cache); + verifyLoadingIsPossible(cache); + } + + @Test + public void testNonEvictableCacheWithWeakInvalidateAll() + throws Exception + { + NonKeyEvictableCache cache = buildNonEvictableCacheWithWeakInvalidateAll(CacheBuilder.newBuilder()); + verifyKeyInvalidationIsImpossible(cache); + verifyClearIsPossible(cache); + verifyLoadingIsPossible(cache); + } + + @Test + public void testNonEvictableLoadingCache() + throws Exception + { + NonEvictableLoadingCache cache = buildNonEvictableCache(CacheBuilder.newBuilder(), CacheLoader.from(key -> key)); + verifyKeyInvalidationIsImpossible(cache); + verifyClearIsImpossible(cache); + verifyLoadingIsPossible(cache); + } + + @Test + public void testNonEvictableLoadingCacheWithWeakInvalidateAll() + throws Exception + { + NonKeyEvictableLoadingCache cache = buildNonEvictableCacheWithWeakInvalidateAll(CacheBuilder.newBuilder(), CacheLoader.from(key -> key)); + verifyKeyInvalidationIsImpossible(cache); + verifyClearIsPossible(cache); + verifyLoadingIsPossible(cache); + } + + private static void verifyLoadingIsPossible(Cache cache) + throws Exception + { + Object key = new Object(); + Object value = new Object(); + // Verify the previous load was inserted into the cache + assertSame(cache.get(key, () -> value), value); + } + + private static void verifyKeyInvalidationIsImpossible(Cache cache) + { + assertThatThrownBy(() -> cache.invalidate(new Object())) + .isInstanceOf(UnsupportedOperationException.class) + .hasMessage("invalidate(key) does not invalidate ongoing loads, so a stale value may remain in the cache for ever. Use EvictableCache if you need invalidation"); + assertThatThrownBy(() -> cache.invalidateAll(ImmutableList.of())) + .isInstanceOf(UnsupportedOperationException.class) + .hasMessage("invalidateAll(keys) does not invalidate ongoing loads, so a stale value may remain in the cache for ever. Use EvictableCache if you need invalidation"); + } + + private static void verifyClearIsImpossible(Cache cache) + { + assertThatThrownBy(cache::invalidateAll) + .isInstanceOf(UnsupportedOperationException.class) + .hasMessage("invalidateAll does not invalidate ongoing loads, so a stale value may remain in the cache for ever. Use EvictableCache if you need invalidation, or use SafeCaches.buildNonEvictableCacheWithWeakInvalidateAll() if invalidateAll is not required for correctness"); + // TODO test asMap().clear() + } + + private static void verifyClearIsPossible(Cache cache) + throws Exception + { + Object key = new Object(); + Object firstValue = new Object(); + cache.get(key, () -> firstValue); + assertSame(cache.getIfPresent(key), firstValue); + + cache.invalidateAll(); + assertThat(cache.getIfPresent(key)).isNull(); + + Object secondValue = new Object(); + cache.get(key, () -> secondValue); + assertSame(cache.getIfPresent(key), secondValue); + cache.asMap().clear(); + assertThat(cache.getIfPresent(key)).isNull(); + } +} From fa11f90b89094b99c63729d9f064fd38c3957627 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Grzegorz=20Kokosi=C5=84ski?= Date: Tue, 25 Oct 2022 15:12:27 +0200 Subject: [PATCH 3/3] Handle map representation of cache in SafeCaches --- .../collect/cache/NonEvictableCacheImpl.java | 24 ++++++++++ .../cache/NonEvictableLoadingCacheImpl.java | 24 ++++++++++ .../cache/NonKeyEvictableCacheImpl.java | 48 +++++++++++++++++++ .../NonKeyEvictableLoadingCacheImpl.java | 45 +++++++++++++++++ .../trino/collect/cache/TestSafeCaches.java | 20 +++++++- 5 files changed, 160 insertions(+), 1 deletion(-) diff --git a/lib/trino-collect/src/main/java/io/trino/collect/cache/NonEvictableCacheImpl.java b/lib/trino-collect/src/main/java/io/trino/collect/cache/NonEvictableCacheImpl.java index 02e9cfd763a2..9a0103f87d20 100644 --- a/lib/trino-collect/src/main/java/io/trino/collect/cache/NonEvictableCacheImpl.java +++ b/lib/trino-collect/src/main/java/io/trino/collect/cache/NonEvictableCacheImpl.java @@ -14,6 +14,9 @@ package io.trino.collect.cache; import com.google.common.cache.Cache; +import com.google.common.collect.ForwardingConcurrentMap; + +import java.util.concurrent.ConcurrentMap; // package-private. The interface provides deprecation and javadoc to help at call sites final class NonEvictableCacheImpl @@ -32,4 +35,25 @@ public void invalidateAll() "Use EvictableCache if you need invalidation, or use SafeCaches.buildNonEvictableCacheWithWeakInvalidateAll() " + "if invalidateAll is not required for correctness"); } + + @Override + public ConcurrentMap asMap() + { + ConcurrentMap map = super.asMap(); + return new ForwardingConcurrentMap() + { + @Override + protected ConcurrentMap delegate() + { + return map; + } + + @Override + public void clear() + { + throw new UnsupportedOperationException("clear() does not invalidate ongoing loads, so a stale value may remain in the cache for ever. " + + "Use EvictableCacheBuilder if you need invalidation"); + } + }; + } } diff --git a/lib/trino-collect/src/main/java/io/trino/collect/cache/NonEvictableLoadingCacheImpl.java b/lib/trino-collect/src/main/java/io/trino/collect/cache/NonEvictableLoadingCacheImpl.java index dcf16f0cac1d..2b093a688fe8 100644 --- a/lib/trino-collect/src/main/java/io/trino/collect/cache/NonEvictableLoadingCacheImpl.java +++ b/lib/trino-collect/src/main/java/io/trino/collect/cache/NonEvictableLoadingCacheImpl.java @@ -14,6 +14,9 @@ package io.trino.collect.cache; import com.google.common.cache.LoadingCache; +import com.google.common.collect.ForwardingConcurrentMap; + +import java.util.concurrent.ConcurrentMap; // package-private. The interface provides deprecation and javadoc to help at call sites final class NonEvictableLoadingCacheImpl @@ -32,4 +35,25 @@ public void invalidateAll() "Use EvictableCache if you need invalidation, or use SafeCaches.buildNonEvictableCacheWithWeakInvalidateAll() " + "if invalidateAll is not required for correctness"); } + + @Override + public ConcurrentMap asMap() + { + ConcurrentMap map = super.asMap(); + return new ForwardingConcurrentMap() + { + @Override + protected ConcurrentMap delegate() + { + return map; + } + + @Override + public void clear() + { + throw new UnsupportedOperationException("clear() does not invalidate ongoing loads, so a stale value may remain in the cache for ever. " + + "Use EvictableCacheBuilder if you need invalidation"); + } + }; + } } diff --git a/lib/trino-collect/src/main/java/io/trino/collect/cache/NonKeyEvictableCacheImpl.java b/lib/trino-collect/src/main/java/io/trino/collect/cache/NonKeyEvictableCacheImpl.java index d52b2acc2002..08502afac67e 100644 --- a/lib/trino-collect/src/main/java/io/trino/collect/cache/NonKeyEvictableCacheImpl.java +++ b/lib/trino-collect/src/main/java/io/trino/collect/cache/NonKeyEvictableCacheImpl.java @@ -15,6 +15,9 @@ import com.google.common.cache.Cache; import com.google.common.cache.ForwardingCache; +import com.google.common.collect.ForwardingConcurrentMap; + +import java.util.concurrent.ConcurrentMap; import static java.util.Objects.requireNonNull; @@ -49,4 +52,49 @@ public void invalidateAll(Iterable keys) throw new UnsupportedOperationException("invalidateAll(keys) does not invalidate ongoing loads, so a stale value may remain in the cache for ever. " + "Use EvictableCache if you need invalidation"); } + + @Override + public ConcurrentMap asMap() + { + ConcurrentMap map = delegate.asMap(); + return new ForwardingConcurrentMap() + { + @Override + protected ConcurrentMap delegate() + { + return map; + } + + @Override + public V remove(Object key) + { + throw new UnsupportedOperationException("remove(key) does not invalidate ongoing loads, so a stale value may remain in the cache for ever. " + + "Use EvictableCacheBuilder if you need invalidation"); + } + + @Override + public boolean remove(Object key, Object value) + { + // defensive forbid, it is not sure if this method is safe to call or not + throw new UnsupportedOperationException("remove(key, value) does not invalidate ongoing loads, so a stale value may remain in the cache for ever. " + + "Use EvictableCacheBuilder if you need invalidation"); + } + + @Override + public boolean replace(K key, V oldValue, V newValue) + { + // defensive forbid, it is not sure if this method is safe to call or not + throw new UnsupportedOperationException("replace(key, oldValue, newValue) does not invalidate ongoing loads, so a stale value may remain in the cache for ever. " + + "Use EvictableCacheBuilder if you need invalidation"); + } + + @Override + public V replace(K key, V value) + { + // defensive forbid, it is not sure if this method is safe to call or not + throw new UnsupportedOperationException("replace(key, value) does not invalidate ongoing loads, so a stale value may remain in the cache for ever. " + + "Use EvictableCacheBuilder if you need invalidation"); + } + }; + } } diff --git a/lib/trino-collect/src/main/java/io/trino/collect/cache/NonKeyEvictableLoadingCacheImpl.java b/lib/trino-collect/src/main/java/io/trino/collect/cache/NonKeyEvictableLoadingCacheImpl.java index 235e4a696b78..a803deae2bca 100644 --- a/lib/trino-collect/src/main/java/io/trino/collect/cache/NonKeyEvictableLoadingCacheImpl.java +++ b/lib/trino-collect/src/main/java/io/trino/collect/cache/NonKeyEvictableLoadingCacheImpl.java @@ -15,6 +15,9 @@ import com.google.common.cache.ForwardingLoadingCache; import com.google.common.cache.LoadingCache; +import com.google.common.collect.ForwardingConcurrentMap; + +import java.util.concurrent.ConcurrentMap; import static java.util.Objects.requireNonNull; @@ -55,4 +58,46 @@ public void invalidateAll(Iterable keys) throw new UnsupportedOperationException("invalidateAll(keys) does not invalidate ongoing loads, so a stale value may remain in the cache for ever. " + "Use EvictableCache if you need invalidation"); } + + @Override + public ConcurrentMap asMap() + { + ConcurrentMap map = delegate.asMap(); + return new ForwardingConcurrentMap() + { + @Override + protected ConcurrentMap delegate() + { + return map; + } + + @Override + public V remove(Object key) + { + throw new UnsupportedOperationException("remove(key) does not invalidate ongoing loads, so a stale value may remain in the cache for ever. " + + "Use EvictableCacheBuilder if you need invalidation"); + } + + @Override + public boolean remove(Object key, Object value) + { + throw new UnsupportedOperationException("remove(key, value) does not invalidate ongoing loads, so a stale value may remain in the cache for ever. " + + "Use EvictableCacheBuilder if you need invalidation"); + } + + @Override + public boolean replace(K key, V oldValue, V newValue) + { + throw new UnsupportedOperationException("replace(key, oldValue, newValue) does not invalidate ongoing loads, so a stale value may remain in the cache for ever. " + + "Use EvictableCacheBuilder if you need invalidation"); + } + + @Override + public V replace(K key, V value) + { + throw new UnsupportedOperationException("replace(key, value) does not invalidate ongoing loads, so a stale value may remain in the cache for ever. " + + "Use EvictableCacheBuilder if you need invalidation"); + } + }; + } } diff --git a/lib/trino-collect/src/test/java/io/trino/collect/cache/TestSafeCaches.java b/lib/trino-collect/src/test/java/io/trino/collect/cache/TestSafeCaches.java index b7b527f33871..041acc734129 100644 --- a/lib/trino-collect/src/test/java/io/trino/collect/cache/TestSafeCaches.java +++ b/lib/trino-collect/src/test/java/io/trino/collect/cache/TestSafeCaches.java @@ -19,6 +19,8 @@ import com.google.common.collect.ImmutableList; import org.testng.annotations.Test; +import java.util.Map; + import static io.trino.collect.cache.SafeCaches.buildNonEvictableCache; import static io.trino.collect.cache.SafeCaches.buildNonEvictableCacheWithWeakInvalidateAll; import static org.assertj.core.api.Assertions.assertThat; @@ -84,6 +86,19 @@ private static void verifyKeyInvalidationIsImpossible(Cache cach assertThatThrownBy(() -> cache.invalidateAll(ImmutableList.of())) .isInstanceOf(UnsupportedOperationException.class) .hasMessage("invalidateAll(keys) does not invalidate ongoing loads, so a stale value may remain in the cache for ever. Use EvictableCache if you need invalidation"); + Object object = new Object(); + assertThatThrownBy(() -> cache.asMap().remove(object)) + .isInstanceOf(UnsupportedOperationException.class) + .hasMessage("remove(key) does not invalidate ongoing loads, so a stale value may remain in the cache for ever. Use EvictableCacheBuilder if you need invalidation"); + assertThatThrownBy(() -> cache.asMap().remove(object, object)) + .isInstanceOf(UnsupportedOperationException.class) + .hasMessage("remove(key, value) does not invalidate ongoing loads, so a stale value may remain in the cache for ever. Use EvictableCacheBuilder if you need invalidation"); + assertThatThrownBy(() -> cache.asMap().replace(object, object)) + .isInstanceOf(UnsupportedOperationException.class) + .hasMessage("replace(key, value) does not invalidate ongoing loads, so a stale value may remain in the cache for ever. Use EvictableCacheBuilder if you need invalidation"); + assertThatThrownBy(() -> cache.asMap().replace(object, object, object)) + .isInstanceOf(UnsupportedOperationException.class) + .hasMessage("replace(key, oldValue, newValue) does not invalidate ongoing loads, so a stale value may remain in the cache for ever. Use EvictableCacheBuilder if you need invalidation"); } private static void verifyClearIsImpossible(Cache cache) @@ -91,7 +106,10 @@ private static void verifyClearIsImpossible(Cache cache) assertThatThrownBy(cache::invalidateAll) .isInstanceOf(UnsupportedOperationException.class) .hasMessage("invalidateAll does not invalidate ongoing loads, so a stale value may remain in the cache for ever. Use EvictableCache if you need invalidation, or use SafeCaches.buildNonEvictableCacheWithWeakInvalidateAll() if invalidateAll is not required for correctness"); - // TODO test asMap().clear() + Map map = cache.asMap(); + assertThatThrownBy(map::clear) + .isInstanceOf(UnsupportedOperationException.class) + .hasMessage("clear() does not invalidate ongoing loads, so a stale value may remain in the cache for ever. Use EvictableCacheBuilder if you need invalidation"); } private static void verifyClearIsPossible(Cache cache)