Ensure caches are not used unsafely #10691

findepi · 2022-01-19T14:12:05Z

Guava's Cache and LoadingCache have concurrency issues around
invalidation and ongoing loads.

Ensure that

code uses EvictableCache or EvictableLoadingCache which fix the
probem, or
code uses safety wrappers, NonEvictableCache,
NonEvictableLoadingCache, which fail when unsafe invalidation is
called. Additionally, the interfaces have the unimplemented methods
marked as @Deprecated, to signal the problem as early as possible.

Follows #10512

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

plugin/trino-hive/src/main/java/io/trino/plugin/hive/CachingDirectoryLister.java

...-raptor-legacy/src/main/java/io/trino/plugin/raptor/legacy/storage/ShardRecoveryManager.java

pom.xml

findepi · 2022-01-19T17:06:49Z

plugin/trino-bigquery/src/main/java/io/trino/plugin/bigquery/BigQueryClient.java

@@ -64,21 +61,12 @@
    private final BigQuery bigQuery;
    private final ViewMaterializationCache materializationCache;
    private final boolean caseInsensitiveNameMatching;
-    private final Cache<String, Optional<RemoteDatabaseObject>> remoteDatasets;
-    private final Cache<TableId, Optional<RemoteDatabaseObject>> remoteTables;


cc @hashhar see Remove unused caches in BigQueryClient.

🤦‍♂️

Indeed. It has been present since the initial commit. I compute a fresh mapping near // explicitly cache the information if the requested dataset doesn't exist but don't put it into the cache at all.

I'd prefer fixing this instead of removing it though (in separate PR).

I'd prefer fixing this instead of removing it though (in separate PR).

understood, except that i didn't want to spend time fixing this particular one.

i can drop the change. note that this is not the only problem, probably -- what about cache invalidation?

Remove unused caches in BigQueryClient dropped

@grantatspothero is both fixing this to actually populate the cache and converting to use the EvictableCache to prevent leaking keys that are already being loaded.

Note that we don't have a procedure to clear the entire cache so it's a better situation than the JDBC connectors.

Let's remove the cache for now. We ran into some isssues - bulk loading not playing well with loading caches, no invalidation logic when we make changes via Trino etc.

@findepi Can you restore your commit? Sorry for the back and forth.

We ran into some isssues - bulk loading not playing well with loading caches, no invalidation logic when we make changes via Trino etc.

@hashhar @grantatspothero
bulk list + cache.put is inherently not playing well with invalidation

in JDBC, we 'solved' this problem by not having invalidation (CachingIdentifierMapping)
cc @kokosing

Can you restore your commit?

#10725

core/trino-main/src/main/java/io/trino/execution/SqlTaskManager.java

losipiuk · 2022-01-19T21:26:12Z

core/trino-main/src/main/java/io/trino/execution/SqlTaskManager.java

@@ -496,6 +497,8 @@ public void removeOldTasks()
                    try {
                        DateTime endTime = taskInfo.getStats().getEndTime();
                        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);


would that make sense to assert that task was actually removed to follow what comment above says.

maybe, but i don't feel comfortable adding assertions in this class. do you want to address this as a followup?

grantatspothero · 2022-01-20T17:02:47Z

@findepi a pattern that often comes up with the connectors is we want a cache that also keeps track of negative cache entries.

For example, in the bigquery connector we can only bulk list tables in the bigquery API to populate the table name cache, so if someone requests reading a table that does not exist we have to list all the tables in the schema. It would be nice to keep track of this nonexistent table so we do not keep hitting the bigquery API.
#10697

Is there a cache implementation we should be using for bulk loads that handles concurrency and invalidation of a negative cache without races?

kokosing

Thanks!

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

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

findepi · 2022-01-21T11:27:56Z

@grantatspothero

For example, in the bigquery connector we can only bulk list tables in the bigquery API to populate the table name cache

bulk list is not bulk load

you don't ask a cache for multiple potentially existing entries at the same time

Is there a cache implementation we should be using for bulk loads that handles concurrency and invalidation of a negative cache without races?

EvictableLoadingCache is supposed to be that (is intended to support bulk loads, invalidation, as used in CachingHiveMetastore for 2 caches).

not sure about "negative cache". from cache perspective, there is no such thing, and "negativeness" is handled by using a marker, right?

let's continue BigQuery caching needs under BigQuery caching PR

findepi · 2022-01-21T11:34:49Z

let's continue BigQuery caching needs under BigQuery caching PR

oh, i thought there is a PR. Let's talk under the issue #10697

They were never written to, so always empty. This also removes `bigquery.case-insensitive-name-matching.cache-ttl` config property, which was accepted, but didn't do anything.

The cache has no `refreshAfterWrite`, so `asyncReloading` does not change anything.

`UniformNodeSelectorFactory` and `TopologyAwareNodeSelectorFactory` attempt to warn only once per 30 seconds about a node being inaccessible. Fix a race around that which allowed multiple threads to issue a warning at once.

The `.getIfPresent() compute .put()` can be replaced with a `.get(key, loader)`.

The cache has no expiration, so it's equivalent to `ConcurrentHashMap` with `Map.computeIfAbsent`.

Guava's `Cache` and `LoadingCache` have concurrency issues around invalidation and ongoing loads. Ensure that - code uses `EvictableCache` or `EvictableLoadingCache` which fix the probem, or - code uses safety wrappers, `NonEvictableCache`, `NonEvictableLoadingCache`, which fail when unsafe invalidation is called. Additionally, the interfaces have the unimplemented methods marked as `@Deprecated`, to signal the problem as early as possible.

findepi · 2022-01-21T12:28:22Z

rebased on top of #10725

findepi requested review from losipiuk and kokosing January 19, 2022 14:12

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

findepi commented Jan 19, 2022

View reviewed changes

github-actions bot added the tests:hive label Jan 19, 2022

findepi force-pushed the findepi/protect-caches branch from 1ea184f to 6eb77b1 Compare January 19, 2022 17:06

findepi commented Jan 19, 2022

View reviewed changes

findepi mentioned this pull request Jan 19, 2022

BigQuery bigquery.case-insensitive-name-matching.cache-ttl configuration is ignored #10697

Closed

findepi force-pushed the findepi/protect-caches branch from 6eb77b1 to 17fa8e1 Compare January 19, 2022 20:31

losipiuk reviewed Jan 19, 2022

View reviewed changes

core/trino-main/src/main/java/io/trino/execution/SqlTaskManager.java Show resolved Hide resolved

losipiuk reviewed Jan 19, 2022

View reviewed changes

findepi force-pushed the findepi/protect-caches branch from 17fa8e1 to 194cdef Compare January 21, 2022 08:50

kokosing approved these changes Jan 21, 2022

View reviewed changes

findepi force-pushed the findepi/protect-caches branch from 194cdef to 2b2c3b4 Compare January 21, 2022 11:20

findepi requested review from losipiuk and hashhar January 21, 2022 11:29

findepi added 8 commits January 21, 2022 12:58

Remove unused caches in BigQueryClient

3d596b2

They were never written to, so always empty. This also removes `bigquery.case-insensitive-name-matching.cache-ttl` config property, which was accepted, but didn't do anything.

Remove redundant asyncReloading wrapper in function listing cache

bacc314

The cache has no `refreshAfterWrite`, so `asyncReloading` does not change anything.

Fix race around warning in node selector factories

81777c1

`UniformNodeSelectorFactory` and `TopologyAwareNodeSelectorFactory` attempt to warn only once per 30 seconds about a node being inaccessible. Fix a race around that which allowed multiple threads to issue a warning at once.

Use lazy computation cache idiom

4649f82

The `.getIfPresent() compute .put()` can be replaced with a `.get(key, loader)`.

Reduce SqlTaskManager.removeOldTasks method visibility

7deb55a

Document safe cache invalidation in SqlTaskManager

bdd55bb

Remove unnecessary cache use in RedisJedisManager

1c1190b

The cache has no expiration, so it's equivalent to `ConcurrentHashMap` with `Map.computeIfAbsent`.

findepi force-pushed the findepi/protect-caches branch from 2b2c3b4 to 9c5c238 Compare January 21, 2022 12:28

github-actions bot added the docs label Jan 21, 2022

findepi merged commit 0747a37 into trinodb:master Jan 21, 2022

findepi deleted the findepi/protect-caches branch January 21, 2022 15:54

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

mosabua mentioned this pull request Jan 21, 2022

Add Trino 369 release notes #10553

Merged

tangjiangling mentioned this pull request Jul 6, 2022

Remove redundant TODO #13098

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure caches are not used unsafely #10691

Ensure caches are not used unsafely #10691

findepi commented Jan 19, 2022

findepi Jan 19, 2022

hashhar Jan 19, 2022 •

edited

Loading

findepi Jan 19, 2022

findepi Jan 19, 2022

hashhar Jan 20, 2022

hashhar Jan 21, 2022

findepi Jan 21, 2022

findepi Jan 21, 2022

losipiuk Jan 19, 2022

findepi Jan 21, 2022

grantatspothero commented Jan 20, 2022 •

edited

Loading

kokosing left a comment

findepi commented Jan 21, 2022

findepi commented Jan 21, 2022

findepi commented Jan 21, 2022

Ensure caches are not used unsafely #10691

Ensure caches are not used unsafely #10691

Conversation

findepi commented Jan 19, 2022

Choose a reason for hiding this comment

hashhar Jan 19, 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

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

grantatspothero commented Jan 20, 2022 • edited Loading

kokosing left a comment

Choose a reason for hiding this comment

findepi commented Jan 21, 2022

findepi commented Jan 21, 2022

findepi commented Jan 21, 2022

hashhar Jan 19, 2022 •

edited

Loading

grantatspothero commented Jan 20, 2022 •

edited

Loading