Fix invalidation race in CachingHiveMetastore by findepi · Pull Request #10646 · trinodb/trino

findepi · 2022-01-17T13:45:10Z

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.

Fixes #10512.

The changed caches do not set `cacheBuilder.refreshAfterWrite` (since 3a1bf59), so the `asyncReloading`, which changes `CacheLoader.reload`, and delegates the reset), was redundant.

findepi · 2022-01-17T15:37:08Z

cc @alexjo2144 @homar

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

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 · 2022-01-18T08:40:55Z

50 successful and 2 failing checks

ci / test (:trino-hive -P test-failure-recovery) (pull_request) -- our new best friend, Flaky TestHiveQueryFailureRecoveryTest(testInsertIntoExistingPartitionBucketed, testInsertIntoNewPartition, testInsertIntoNewPartitionBucketed, testReplaceExistingPartition) #10631
ci / pt (default, suite-tpcds, 11) (pull_request) -- setup-java failed.

...in-toolkit/src/test/java/io/trino/plugin/base/security/TestFileBasedSystemAccessControl.java

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

kokosing · 2022-01-18T08:55:23Z

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

ImmutableBiMap?

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

kokosing · 2022-01-18T08:58:11Z

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

values.entrySet().stream()...collect()?

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.

if it is a case then maybe a comment?

where would you see that?

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

kokosing · 2022-01-18T09:08:40Z

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

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.

Just boiler plate. Suppressing something is cheaper that writing a class.

Fix grammar. Improve wording.

findepi · 2022-01-18T09:54:33Z

AC (https://github.com/trinodb/trino/compare/d7b33c87ef255b66edda68df267906a56c9cdf7c..3e8f373864f1fa30e2ef6c92c70abf7b0b3a665b)

losipiuk · 2022-01-18T10:50:54Z

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

nit:

List<K> keys = tokenList.stream() .map(Token::getKey) .collect(toCollection(ArrayList::new));

see #10646 (comment)

kokosing

% I haven't reviewed testLoadAfterInvalidate.

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

kokosing · 2022-01-18T11:02:04Z

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

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.

right, changing.

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.

Remove redundant asyncReloading CacheLoader wrapper

56daff8

The changed caches do not set `cacheBuilder.refreshAfterWrite` (since 3a1bf59), so the `asyncReloading`, which changes `CacheLoader.reload`, and delegates the reset), was redundant.

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

findepi force-pushed the findepi/hive-cache-soundly branch from 655fb71 to a0a9d14 Compare January 17, 2022 14:01

findepi added the tests:hive label Jan 17, 2022

findepi force-pushed the findepi/hive-cache-soundly branch from a0a9d14 to 6d7de31 Compare January 17, 2022 15:36

findepi marked this pull request as ready for review January 17, 2022 15:36

findepi requested review from kokosing and losipiuk January 17, 2022 15:36

findepi force-pushed the findepi/hive-cache-soundly branch from 6d7de31 to ec2ffe3 Compare January 17, 2022 17:03

Extract testing enum for reuse

edf0ee1

findepi force-pushed the findepi/hive-cache-soundly branch 2 times, most recently from c96e83f to b6ebad4 Compare January 17, 2022 18:07

findepi added 3 commits January 17, 2022 22:43

Extract CacheStatsAssertions for reuse

2c5dcb6

Extract `CacheStatsAssertions` from `TestCachingJdbcClient`.

Test EvictableCache hits

5125362

findepi force-pushed the findepi/hive-cache-soundly branch from b6ebad4 to f3dca68 Compare January 17, 2022 21:45

kokosing reviewed Jan 18, 2022

View reviewed changes

Fix and improve EvictableCache javadoc

e454a50

Fix grammar. Improve wording.

findepi force-pushed the findepi/hive-cache-soundly branch from d7b33c8 to 3e8f373 Compare January 18, 2022 09:54

findepi requested a review from kokosing January 18, 2022 09:54

losipiuk reviewed Jan 18, 2022

View reviewed changes

kokosing approved these changes Jan 18, 2022

View reviewed changes

findepi added 2 commits January 18, 2022 12:41

Improve test interleaving determinism

fcce46e

Fix invalidation race in CachingHiveMetastore

0e42ed3

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 January 18, 2022 11:41

losipiuk approved these changes Jan 18, 2022

View reviewed changes

findepi merged commit 4c99cff into trinodb:master Jan 18, 2022

findepi deleted the findepi/hive-cache-soundly branch January 18, 2022 21:36

findepi mentioned this pull request Jan 18, 2022

Release notes for 369 #10552

Closed

mosabua mentioned this pull request Jan 19, 2022

Add Trino 369 release notes #10553

Merged

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

Conversation

findepi commented Jan 17, 2022

Uh oh!

findepi commented Jan 17, 2022

Uh oh!

findepi commented Jan 18, 2022

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

findepi commented Jan 18, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kokosing left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants