Fix flush_metadata_cache with Glue v2 #22075
Conversation
findepi
force-pushed
the
findepi/hive-glue-flush
branch
from
d98206c to
a5be3f6
Compare
|
currently based on #22068 |
findepi
force-pushed
the
findepi/hive-glue-flush
branch
from
a5be3f6 to
2a7c816
Compare
findepi
force-pushed
the
findepi/hive-glue-flush
branch
3 times, most recently
from
96b14ad to
1b0dd8e
Compare
|
tests failed. problematic test refactor extracted to #22079 |
| private enum Global | ||
| { | ||
| GLOBAL | ||
| } |
There was a problem hiding this comment.
I thought we allowed single line enums when there is one value. Is that not allowed?
There was a problem hiding this comment.
i can drop this change, if you want. but i want intellij formatter to not change the code, so if it does, we either reformat the code or update the airlift codestyle
|
|
||
| @Inject | ||
| public FlushMetadataCacheProcedure(Optional<CachingHiveMetastore> cachingHiveMetastore) | ||
| public FlushMetadataCacheProcedure(Optional<CachingHiveMetastore> cachingHiveMetastore, Optional<GlueCache> glueCache) |
There was a problem hiding this comment.
I suggest we add a single interface for this, say MetastoreCacheControl or FlushMetastoreCache or something. Then this code could be generic again.
There was a problem hiding this comment.
can be. let's do this as a follow-up
| CachingHiveMetastore cachingHiveMetastore = this.cachingHiveMetastore | ||
| .orElseThrow(() -> new TrinoException(HiveErrorCode.HIVE_METASTORE_ERROR, "Cannot flush, metastore cache is not enabled")); | ||
| if (cachingHiveMetastore.isEmpty() && glueCache.isEmpty()) { | ||
| // TODO this currently does not work. CachingHiveMetastore is always bound for metastores other than Glue, even when caching is disabled, |
There was a problem hiding this comment.
Using an optional binder on a "flush" interface may be a way to avoid this problem. We would just need to do bind the interface when shared cache is enabled or glue cache is enabled.
There was a problem hiding this comment.
i actually don't think we can restore this logic now.
people could become dependent on the fact that they can call flush any time they want. it would be a breaking change.
i didn't want to deal with this within this PR, hence only a TODO added
| @@ -17,9 +17,9 @@ | |||
|
|
|||
| import static java.util.Objects.requireNonNull; | |||
|
|
|||
| record PartitionName(List<String> partitionValues) | |||
| public record PartitionName(List<String> partitionValues) | |||
There was a problem hiding this comment.
I have been thinking we should add this interface to our general Hive model, and update usages. It just make is so much easier to understand what is going on.
Follow-up to 0ab49b4.
It's generally more expected to ask for information once, so favor `.add(...)` over `.addCopies(..., 1)`, so that repeated information retrieval stands out.
Similar to a test we have for Iceberg (`TestIcebergGlueCatalogAccessOperations`). Hive and Iceberg currently have different Glue implementations and they also have different metastore/catalog access patterns, so both need to be tested to prevent regressions.
Hive tests have packages for thrift, file, glue metastores' tests. Move the tests from outside into these packages.
1. It's misnamed: it's singular, while the corresponding operation operates on multiple partition names (in batches) 2. It's redundant: the actual call counted by getPartitions.
`flush_metadata_cache` worked with generic `CachingHiveMetastore` and was tested once, orthogonally to selected metastore. Glue v2 implementation has a dedicated own caching implementation, so `flush_metadata_cache` has to be updated.
findepi
force-pushed
the
findepi/hive-glue-flush
branch
from
1b0dd8e to
27cb39d
Compare
flush_metadata_cacheworked with genericCachingHiveMetastoreandwas tested once, orthogonally to selected metastore. Glue v2
implementation has a dedicated own caching implementation, so
flush_metadata_cachehas to be updated.