Fix invalidation race in CachingHiveMetastore #10646
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The changed caches do not set `cacheBuilder.refreshAfterWrite` (since 3a1bf59), so the `asyncReloading`, which changes `CacheLoader.reload`, and delegates the reset), was redundant.
findepi
force-pushed
the
findepi/hive-cache-soundly
branch
from
655fb71
to
a0a9d14
Compare
findepi
force-pushed
the
findepi/hive-cache-soundly
branch
from
a0a9d14
to
6d7de31
Compare
Capture more commonalities that before. - `asyncReloading` is used for all caches with `refreshMillis` set - `memoizeMetastore` no longer provide an (unused) Executor - individual cache instantiation is now single line, so all the code reads better - separate bulk-loading cache building from non-bulks loading, since we do not want to do refreshes for bulk-loading caches - remove redundant `cacheBuilder = cacheBuilder.xxx` assignments
findepi
force-pushed
the
findepi/hive-cache-soundly
branch
from
6d7de31
to
ec2ffe3
Compare
findepi
force-pushed
the
findepi/hive-cache-soundly
branch
2 times, most recently
from
c96e83f
to
b6ebad4
Compare
They have same names as test resources in trino-main. This allows toolkit's and main's test jars to be used together on a plugin's classpath. The chosen prefix, `file-based-system-`, is already used for a few other test resources used in the same test class.
Extract `CacheStatsAssertions` from `TestCachingJdbcClient`.
findepi
force-pushed
the
findepi/hive-cache-soundly
branch
from
b6ebad4
to
f3dca68
Compare
|
|
kokosing
reviewed
Jan 18, 2022
...in-toolkit/src/test/java/io/trino/plugin/base/security/TestFileBasedSystemAccessControl.java
Show resolved
Hide resolved
lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/cache/EvictableLoadingCache.java
Outdated
Show resolved
Hide resolved
| public ImmutableMap<K, V> getAll(Iterable<? extends K> keys) | ||
| throws ExecutionException | ||
| { | ||
| BiMap<K, Token<K>> keyToToken = HashBiMap.create(); |
There was a problem hiding this comment.
I would expect ImmutableBiMap to be less performant, since it tries to create a compacted version at the build() time.
It's a method local state, so immutability doesn't buy me anything
|
|
||
| BiMap<Token<K>, K> tokenToKey = keyToToken.inverse(); | ||
| ImmutableMap.Builder<K, V> result = ImmutableMap.builder(); | ||
| for (Map.Entry<Token<K>, V> entry : values.entrySet()) { |
There was a problem hiding this comment.
values.entrySet().stream()...collect()?
There was a problem hiding this comment.
I am implementing a generic-purpose class and i don't know where it's going to be used.
I avoid Streams in this code for performance reasons. We know that Streams are not suitable for every occasion.
There was a problem hiding this comment.
if it is a case then maybe a comment?
There was a problem hiding this comment.
where would you see that?
lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/cache/EvictableLoadingCache.java
Outdated
Show resolved
Hide resolved
lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/cache/EvictableLoadingCache.java
Show resolved
Hide resolved
| } | ||
|
|
||
| @SuppressModernizer | ||
| private static Integer newInteger(int value) |
There was a problem hiding this comment.
I would prefer a dedicated class. It would be known what is the goal of it. No need to copy paste, no need for SuppressWarnings. Class could have a javadoc etc.
There was a problem hiding this comment.
Just boiler plate. Suppressing something is cheaper that writing a class.
Fix grammar. Improve wording.
findepi
force-pushed
the
findepi/hive-cache-soundly
branch
from
d7b33c8
to
3e8f373
Compare
losipiuk
reviewed
Jan 18, 2022
Comment on lines
+353
to
+351
| List<K> keys = new ArrayList<>(); | ||
| for (Token<K> token : tokenList) { | ||
| keys.add(token.getKey()); | ||
| } |
There was a problem hiding this comment.
nit:
List<K> keys = tokenList.stream()
.map(Token::getKey)
.collect(toCollection(ArrayList::new));
kokosing
approved these changes
Jan 18, 2022
lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/cache/EvictableLoadingCache.java
Show resolved
Hide resolved
| @Override | ||
| public void invalidateAll() | ||
| { | ||
| Set<K> keys = ImmutableSet.copyOf(tokensCache.asMap().keySet()); |
There was a problem hiding this comment.
Should we invalidate all entries from dataCache that do not are not mentioned in tokens? So we release stale entries of dataCache in the opportunistic way.
The `LoadingCache` used in `CachingHiveMetastore` 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.
findepi
force-pushed
the
findepi/hive-cache-soundly
branch
from
3e8f373
to
0e42ed3
Compare
losipiuk
approved these changes
Jan 18, 2022
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The
LoadingCacheused inCachingHiveMetastorewas susceptible to a racebetween load-in-flight and invalidation. The invalidation of a key would
not prevent load in flight from being inserted into the cache.
Fixes #10512.