Fix CachingHiveMetastore.getPartitionsByNames failing while cache invalidation

[findepi]

CachingHiveMetastore uses HivePartitionName for keys in partition
cache. HivePartitionName has a peculiar, semi- value-based equality.
HivePartitionName may or may not be missing a name and it matters for
bulk load, but it doesn't matter for a single-partition load. Because
of equality semantics, the cache keys may gets mixed during bulk load.
This happens within EvictableCache.getAll where we compute tokens. We
may get an existing token and normally it's fine, since existing entry
is satisfied without a bulk load. However, if existing token has no data
in the EvictableCache.dataCache (e.g. due to explicit or implicit
invalidation), the loading gets invoked. Thus, a bulk loading may get a
HivePartitionName without a name, which is unexpected (and
unsupported) in the bulk loading.

Fixes #12513

[findepi]

This is just a test at this moment, not a solution.

My favorite solution would be to kill this hack

trino/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/HivePartitionName.java

Line 35 in 03d8e5a

private final Optional<String> partitionName; // does not participate in hashCode/equals

private final Optional<String> partitionName; // does not participate in hashCode/equals

since it caused me too much problems already.

cc @sopel39 @losipiuk @electrum

[findinpath]

@findepi can you attach to the description of the PR a series of SQL statements that reproduce the scenario?

[findepi]

@findepi can you attach to the description of the PR a series of SQL statements that reproduce the scenario?

No. It cannot be observed in sequential process, unless there is a very specific timing.
See however TestCachingHiveMetastore, it should convey information about the problematic scenario well.

[losipiuk]

LGTM (just skimmed tests)

[findepi]

AC

[findepi]

The change broke load sharing for concurrent single-value loads. Will fix address that.

[findepi]

The change broke load sharing for concurrent single-value loads. Will fix address that.

Added a test and retracted change in EvictableCache.get.
The EvictableCache.getAll changes are retained, but modified a bit.
For example, the dangling tokens are cleaned up now.

[findepi]

cc @electrum @martint if you want to indulge yourself with some concurrency related stuff

[martint]

Thanks for the detailed explanation of the problem in the commit message. Would you mind describing, at a high level, the approach to fixing it?

[martint]

We should look into jcstress (https://github.com/openjdk/jcstress) for testing concurrency (for later, not as part of this PR).

[findepi]

Thanks for the detailed explanation of the problem in the commit message. Would you mind describing, at a high level, the approach to fixing it?

Well, the fix is the inverse of the problem -- pass the keys provided to getAll further to loadAll, without "exchanging" them into equal (but not same) keys based on cache state.