Fix invalidation race in CachingJdbcClient #10544

findepi · 2022-01-11T16:20:28Z

The Cache used in CachingJdbcClient was susceptible to a race
between load-in-flight and invalidation. The invalidation of a key would
not prevent load in flight from being inserted into the cache.

For #10512, other caches need to follow.

lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/cache/EvictableCache.java

losipiuk · 2022-01-12T11:53:14Z

lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/cache/EvictableCache.java

+
+    public EvictableCache(CacheBuilder<? super K, Object> cacheBuilder)
+    {
+        this.delegate = cacheBuilder.build();


Should you assert that delegate is not a LoadingCache?

you get LoadingCache from builder.build(loader) right?

losipiuk · 2022-01-12T11:54:53Z

lib/trino-plugin-toolkit/src/test/java/io/trino/plugin/base/cache/TestEvictableCache.java

+    public void testInvalidateOngoingLoad(Invalidation invalidation)
+            throws Exception
+    {
+        Cache<Integer, String> cache = EvictableCache.buildWith(CacheBuilder.newBuilder());


Add

public static <K, V> EvictableCache<K, V> build() { return new EvictableCache<>(CacheBuilder.newBuilder()); }

?

That's not a useful cache, as it would be unbounded, so i won't add it in EvictableCache.

In the test itself, it doesn't matter.

losipiuk

LGTM

kokosing · 2022-01-12T13:12:32Z

core/trino-main/src/main/java/io/trino/server/security/oauth2/OAuth2TokenExchange.java

@@ -91,6 +91,8 @@ public void setTokenExchangeError(String authIdHash, String message)



What about:

MongoSession

CachingHiveMetastore

CachingJdbcClient
?

MongoSession

covered here

CachingHiveMetastore

tbd

CachingJdbcClient

covered here

kokosing · 2022-01-12T13:13:23Z

core/trino-main/src/main/java/io/trino/server/security/oauth2/OAuth2TokenExchange.java

@@ -91,6 +91,8 @@ public void setTokenExchangeError(String authIdHash, String message)

    public void dropToken(UUID authId)
    {
+        // TODO this may not invalidate ongoing loads (https://github.com/trinodb/trino/issues/10512, https://github.com/google/guava/issues/1881).


s/may/does/

Guava can change. I want the reader to be thoughtful, not take my words for granted.

kokosing · 2022-01-12T13:16:48Z

lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/cache/EvictableCache.java

+ * A Cache implementation similar to ones produced by {@code com.google.common.cache.CacheBuilder},
+ * but one that does not exhibits <a href="https://github.com/google/guava/issues/1881">Guava issue #1881</a>, i.e.
+ * a {@link #getIfPresent(Object)} after {@link #invalidate(Object)} is guaranteed to return {@code null} and
+ * {@link #get(Object, Callable)} after {@link #invalidate(Object)} is guaranteed to load a fresh value.


Please mention about invalidateAll. And mention that this cache will return a key for object that for which load is in-flight.

This is only meant as examples, not full log of differences.

lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/cache/EvictableCache.java

lib/trino-plugin-toolkit/src/test/java/io/trino/plugin/base/cache/TestEvictableCache.java

lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/cache/EvictableCache.java

kokosing · 2022-01-12T13:37:58Z

lib/trino-plugin-toolkit/src/test/java/io/trino/plugin/base/cache/TestEvictableCache.java

+            assertEquals((long) cache.get(key, remoteState::get), remoteState.get());
+        }
+        finally {
+            executor.shutdownNow();


I would keep executor as a field, that way each test would be less code would be less polluted.

since threads are paramount here, i'd prefer the tests not to share the executor

kokosing · 2022-01-12T13:39:39Z

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/CachingJdbcClient.java

-        Optional<JdbcTableHandle> tableHandle = delegate.getTableHandle(session, schemaTableName);
-        if (tableHandle.isPresent() || cacheMissing) {
-            tableHandleCache.put(key, tableHandle);
+            if (cacheMissing || cachedTableHandle.isPresent()) {


I would move this to different preparatory commit.

It's related.

kokosing · 2022-01-12T13:42:50Z

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/CachingJdbcClient.java

@@ -547,7 +545,7 @@ CacheStats getStatisticsCacheStats()

    private static <T, V> void invalidateCache(Cache<T, V> cache, Predicate<T> filterFunction)


simply pass EvictableCache as argument here

You mean to remove the cast?
I want to return the code back to interface-based only, when TODO in io.trino.plugin.base.cache.EvictableCache#asMap is resolved.

The `Cache` used in `CachingJdbcClient` was susceptible to a race between load-in-flight and invalidation. The invalidation of a key would not prevent load in flight from inserting an already stale value into the cache. The `TestCachingJdbcClient` test is courtesy by kokosing. Co-authored-by: Grzegorz Kokosiński <[email protected]>

findepi · 2022-01-12T17:12:13Z

AC

kokosing · 2022-01-13T08:17:53Z

Thank you!

findepi requested review from losipiuk and kokosing January 11, 2022 16:20

cla-bot bot added the cla-signed label Jan 11, 2022

findepi force-pushed the findepi/better-cache branch from 0488f80 to 90760b8 Compare January 11, 2022 16:20

findepi changed the title ~~Fix invalidating race in CachingJdbcClient~~ Fix invalidation race in CachingJdbcClient Jan 11, 2022

findepi force-pushed the findepi/better-cache branch 5 times, most recently from 30d864e to 63e0f15 Compare January 11, 2022 19:59

Document safe & unsafe Cache invalidations

0b48946

findepi force-pushed the findepi/better-cache branch from 63e0f15 to c67d35f Compare January 12, 2022 08:41

losipiuk reviewed Jan 12, 2022

View reviewed changes

lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/cache/EvictableCache.java Outdated Show resolved Hide resolved

losipiuk reviewed Jan 12, 2022

View reviewed changes

lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/cache/EvictableCache.java Show resolved Hide resolved

losipiuk reviewed Jan 12, 2022

View reviewed changes

lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/cache/EvictableCache.java Outdated Show resolved Hide resolved

losipiuk reviewed Jan 12, 2022

View reviewed changes

losipiuk approved these changes Jan 12, 2022

View reviewed changes

kokosing approved these changes Jan 12, 2022

View reviewed changes

findepi and others added 2 commits January 12, 2022 18:11

Fix invalidation race in MongoSession

a10b1e5

findepi force-pushed the findepi/better-cache branch from c67d35f to a10b1e5 Compare January 12, 2022 17:12

findepi merged commit 326548d into trinodb:master Jan 13, 2022

findepi deleted the findepi/better-cache branch January 13, 2022 08:12

This was referenced Jan 13, 2022

Release notes for 369 #10552

Closed

Metadata caching may return incorrect data #10512

Closed

github-actions bot added this to the 369 milestone Jan 13, 2022

mosabua mentioned this pull request Jan 13, 2022

Add Trino 369 release notes #10553

Merged

findepi mentioned this pull request Feb 3, 2022

Mark tests that write using Hive as flaky in TestHiveStorageFormats #10926

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix invalidation race in CachingJdbcClient #10544

Fix invalidation race in CachingJdbcClient #10544

findepi commented Jan 11, 2022

losipiuk Jan 12, 2022

findepi Jan 12, 2022

losipiuk Jan 12, 2022

findepi Jan 12, 2022

losipiuk left a comment

kokosing Jan 12, 2022

findepi Jan 12, 2022

kokosing Jan 12, 2022

findepi Jan 12, 2022

kokosing Jan 12, 2022

findepi Jan 12, 2022

kokosing Jan 12, 2022

findepi Jan 12, 2022 •

edited

Loading

kokosing Jan 12, 2022

findepi Jan 12, 2022

kokosing Jan 12, 2022

findepi Jan 12, 2022

findepi commented Jan 12, 2022

kokosing commented Jan 13, 2022

		@@ -91,6 +91,8 @@ public void setTokenExchangeError(String authIdHash, String message)

		@@ -547,7 +545,7 @@ CacheStats getStatisticsCacheStats()

		private static <T, V> void invalidateCache(Cache<T, V> cache, Predicate<T> filterFunction)

Fix invalidation race in CachingJdbcClient #10544

Fix invalidation race in CachingJdbcClient #10544

Conversation

findepi commented Jan 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

losipiuk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

findepi Jan 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

findepi commented Jan 12, 2022

kokosing commented Jan 13, 2022

findepi Jan 12, 2022 •

edited

Loading