HIVE-29281: Make proactive cache eviction work with catalog#6379
HIVE-29281: Make proactive cache eviction work with catalog#6379Neer393 wants to merge 1 commit intoapache:masterfrom
Conversation
asf-ci-hive
added
tests pending
tests unstable
and removed
tests failed
tests pending
labels
asf-ci-hive
added
tests pending
tests passed
and removed
tests unstable
tests pending
labels
|
@zhangbutao I need a review here. I looked at all the merged PRs under HIVE-22820 and have made changes accordingly for making it catalog aware. Please help me here if I missed anything. Thanks |
There was a problem hiding this comment.
Pull request overview
This PR makes LLAP proactive cache eviction catalog-aware by propagating catalog names through cache tags and eviction requests, preventing collisions when identical db/table names exist across catalogs.
Changes:
- Extend LLAP proactive eviction request structure to include catalog scoping (catalog → db → table → partitions).
- Introduce catalog tracking on
TableDesc/PartitionDescand update cache-tag generation to include catalog-qualified names. - Update LLAP cache metadata serialization and unit tests to reflect catalog-qualified cache tags.
Reviewed changes
Copilot reviewed 31 out of 31 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| storage-api/src/java/org/apache/hadoop/hive/common/io/CacheTag.java | Updates cache tag semantics/docs and parent-tag derivation to preserve catalog prefix. |
| ql/src/java/org/apache/hadoop/hive/ql/plan/TableDesc.java | Adds catalogName field and updates constructors/clone to carry catalog without polluting EXPLAIN. |
| ql/src/java/org/apache/hadoop/hive/ql/plan/PartitionDesc.java | Exposes catalog name via PartitionDesc based on TableDesc, with default fallback. |
| ql/src/java/org/apache/hadoop/hive/llap/ProactiveEviction.java | Makes eviction requests catalog-scoped and includes catalog in proto requests and tag matching. |
| llap-common/src/protobuf/LlapDaemonProtocol.proto | Adds catalog name field to EvictEntityRequestProto. |
| ql/src/java/org/apache/hadoop/hive/llap/LlapHiveUtils.java | Prefixes cache tags with catalog when deriving metrics tags. |
| llap-server/src/java/org/apache/hadoop/hive/llap/io/api/impl/LlapIoImpl.java | Adjusts eviction debug logging for catalog+db structure. |
| llap-server/src/java/org/apache/hadoop/hive/llap/io/api/impl/LlapCacheMetadataSerializer.java | Adds backward-ish handling for cache tags missing catalog during decode. |
| llap-server/src/java/org/apache/hadoop/hive/llap/io/metadata/OrcFileEstimateErrors.java | Updates synthetic tag to include default catalog prefix. |
| ql/src/java/org/apache/hadoop/hive/ql/ddl/** | Ensures eviction builders are invoked with catalog where available. |
| ql/src/java/org/apache/hadoop/hive/ql/exec/Utilities.java | Ensures TableDesc created from Table carries catalog name. |
| ql/src/java/org/apache/hadoop/hive/ql/plan/PlanUtils.java | Updates TableDesc construction call sites for new signature. |
| Various test files | Update existing tests and add new coverage for catalog-aware eviction and proto round-trips. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /** | ||
| * Add a partition of a table scoped to the given catalog. | ||
| */ | ||
| public Builder addPartitionOfATable(String catalog, String db, String tableName, | ||
| LinkedHashMap<String, String> partSpec) { | ||
| ensureTable(catalog, db, tableName); | ||
| entities.get(catalog).get(db).get(tableName).add(partSpec); | ||
| return this; |
There was a problem hiding this comment.
Request.Builder claims the catalog key defaults to Warehouse.DEFAULT_CATALOG_NAME, but the builder currently stores the catalog parameter as-is. If a caller passes null (or an empty string), this will create a null key and later NPE in toProtoRequests() when calling toLowerCase(). Normalize catalog (and arguably db/table) at the builder boundary, e.g. default null/blank catalog to the default catalog and enforce non-null keys.
There was a problem hiding this comment.
All calls made to this will never be null.
llap-server/src/java/org/apache/hadoop/hive/llap/io/api/impl/LlapCacheMetadataSerializer.java
Outdated
Show resolved
Hide resolved
asf-ci-hive
added
tests pending
tests passed
and removed
tests passed
tests pending
labels
|
@zhangbutao resolved the copilot's reviews |
Thanks for pinging me. I will do the code review later. |
itests/hive-jmh/src/main/java/org/apache/hive/benchmark/ql/exec/KryoBench.java
Outdated
Show resolved
Hide resolved
llap-server/src/java/org/apache/hadoop/hive/llap/io/api/impl/LlapCacheMetadataSerializer.java
Outdated
Show resolved
Hide resolved
llap-server/src/java/org/apache/hadoop/hive/llap/io/api/impl/LlapIoImpl.java
Outdated
Show resolved
Hide resolved
llap-server/src/java/org/apache/hadoop/hive/llap/io/metadata/OrcFileEstimateErrors.java
Outdated
Show resolved
Hide resolved
llap-server/src/test/org/apache/hadoop/hive/llap/cache/TestCacheContentsTracker.java
Outdated
Show resolved
Hide resolved
llap-server/src/test/org/apache/hadoop/hive/llap/cache/TestCacheContentsTracker.java
Outdated
Show resolved
Hide resolved
|
Made changes as per requested. |
asf-ci-hive
added
tests pending
tests passed
and removed
tests passed
tests pending
labels
asf-ci-hive
added
tests pending
tests passed
and removed
tests passed
tests pending
labels
|
Fixed sonarqube issues as well. All good for approval and merge @zhangbutao @deniskuzZ |
zhangbutao
left a comment
zhangbutao
left a comment
There was a problem hiding this comment.
+1
I think most of This PR is fine. Thanks.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 24 out of 24 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public String getCatalogName() { | ||
| return catalogName; | ||
| } | ||
|
|
||
| public void setCatalogName(String catalogName) { | ||
| this.catalogName = catalogName == null ? Warehouse.DEFAULT_CATALOG_NAME : catalogName; | ||
| } |
There was a problem hiding this comment.
TableDesc.catalogName can remain null for instances built via the no-arg constructor + setters, even though the new constructor/setter normalize null to Warehouse.DEFAULT_CATALOG_NAME. Consider initializing catalogName eagerly (field initializer or in TableDesc()) so getCatalogName() never returns null. Also, equals()/hashCode() currently ignore catalogName, which can cause different-catalog descriptors to compare equal and collide in hash-based collections; include catalogName in both (or document why it must be excluded).
| * the same DB name, and that getSingleCatalogName/getSingleDbName return null when multiple | ||
| * catalog-DB pairs are present. |
There was a problem hiding this comment.
This test’s Javadoc mentions getSingleCatalogName/getSingleDbName, but those methods no longer exist (they were replaced by hasCatalogName/hasDatabaseName). Update the comment to reflect the current API/behavior to avoid confusion for future maintainers.
| * the same DB name, and that getSingleCatalogName/getSingleDbName return null when multiple | |
| * catalog-DB pairs are present. | |
| * the same DB name, and that requests spanning multiple catalog-DB pairs are not treated as | |
| * having a single catalog or database; callers should use hasCatalogName/hasDatabaseName with | |
| * explicit values instead. |
|
|
||
| public static CacheTag cacheTagBuilder(String dbAndTable, String... partitions) { | ||
| String[] parts = dbAndTable.split("\\."); | ||
| if(parts.length < 3) { |
| partDescs.put("mutli=one", "one=/1"); | ||
| partDescs.put("mutli=two/", "two=2"); | ||
| tag = CacheTag.build("math.rules", partDescs); | ||
| tag = CacheTag.build(Warehouse.DEFAULT_CATALOG_NAME + ".math.rules", partDescs); |
There was a problem hiding this comment.
why not handle DEFAULT_CATALOG_NAME inside CacheTag.build when size ==2?
you wouldn't need CacheTag.build(Warehouse.DEFAULT_CATALOG_NAME, "math.rules") either
There was a problem hiding this comment.
The same reason as this - #6379 (comment)
| if (part == null) { | ||
| return CacheTag.build(LlapUtil.getDbAndTableNameForMetrics(path, includeParts)); | ||
| return CacheTag.build( | ||
| Warehouse.DEFAULT_CATALOG_NAME, LlapUtil.getDbAndTableNameForMetrics(path, includeParts)); |
There was a problem hiding this comment.
should we handle path with catalog?
seems that we always go with default catalog when partitionDesc is null. would it ever be non-null for unpartitioned tables?
There was a problem hiding this comment.
Yes I understand but according to the current usages, all of them pass a partition.
Coming to your question of non partitioned tables, I tried but apart from that there is no way of determining the catalog. Could you suggest something ?
|
What changes were proposed in this pull request?
Made the proactive cache eviction catalog aware by making changes in ProactiveCacheEviction file and the CacheTag file.
Why are the changes needed?
The proactive cache eviction should be catalog aware otherwise same name tables under different catalogs may cause false cache hits/miss. To avoid this, the cache eviction should be aware of the catalog.
Does this PR introduce any user-facing change?
No user facing changes as user does not know about the proactive cache eviction.
How was this patch tested?
Added unit tests for with and without catalog and all of them passed. Not sure how to manually test proactive cache eviction so verified only via unit tests