github-actions[bot] commented on code in PR #66260:
URL: https://github.com/apache/doris/pull/66260#discussion_r3680065490
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/hive/HiveMetaStoreCache.java:
##########
@@ -529,21 +529,40 @@ public void invalidateTableCache(NameMapping nameMapping)
{
}
public void invalidatePartitionCache(ExternalTable dorisTable, String
partitionName) {
- NameMapping nameMapping = dorisTable.getOrBuildNameMapping();
+ invalidatePartitionCache(dorisTable.getOrBuildNameMapping(),
partitionName);
+ }
+
+ public void invalidatePartitionCache(NameMapping nameMapping, String
partitionName) {
long id = Util.genIdByName(nameMapping.getLocalDbName(),
nameMapping.getLocalTblName());
- PartitionValueCacheKey key = new PartitionValueCacheKey(nameMapping,
null);
- HivePartitionValues partitionValues =
partitionValuesCache.getIfPresent(key);
- if (partitionValues != null) {
- Long partitionId =
partitionValues.partitionNameToIdMap.get(partitionName);
- List<String> values =
partitionValues.partitionValuesMap.get(partitionId);
- PartitionCacheKey partKey = new PartitionCacheKey(nameMapping,
values);
- HivePartition partition = partitionCache.getIfPresent(partKey);
- if (partition != null) {
- fileCacheRef.get().invalidate(new FileCacheKey(id,
partition.getPath(),
- null, partition.getPartitionValues()));
- partitionCache.invalidate(partKey);
- }
+ // Derive the partition values directly from the partition name (the
partition-values cache is
+ // populated the same way, via HiveUtil.toPartitionValues) instead of
reading them from that
+ // cache, so file cache invalidation still runs when the table's
partition-values cache entry has
+ // been evicted while its file listings are still cached.
+ List<String> values = HiveUtil.toPartitionValues(partitionName);
+
+ PartitionCacheKey partKey = new PartitionCacheKey(nameMapping, values);
+ HivePartition partition = partitionCache.getIfPresent(partKey);
+ LoadingCache<FileCacheKey, FileCacheValue> fileCache =
fileCacheRef.get();
+ if (partition == null) {
+ // Partition metadata cache miss: the exact FileCacheKey cannot be
rebuilt here because it
+ // needs the partition path and input format carried by
HivePartition. Invalidate this
+ // table's cached file listings for the partition by (table id +
partition values). Scoping
+ // by table id is intentional: matching partition values alone
would also drop other tables'
+ // listings that merely share the same partition value names (e.g.
dt=...) at a different
+ // location, forcing needless re-listing. The exact-key path below
(taken when the partition
+ // is cached) already clears a listing regardless of which table
id populated it.
+ fileCache.asMap().keySet().forEach(k -> {
Review Comment:
[P2] Batch miss invalidations before scanning the file cache
`refreshAffectedPartitionsCache` calls this method once per modified
partition, and every `partitionCache` miss now walks the entire file cache
here. With the default 10,000-entry file cache, an insert that modifies 10,000
evicted/uncached partitions performs about 100 million key visits synchronously
after commit, before edit logging, and followers repeat the work during replay.
Please collect the missing partitions' value lists and traverse/index the file
cache once for the batch instead of doing O(modified partitions * file-cache
entries) work; the current one-partition test does not cover this scaling path.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/hive/HiveMetaStoreCache.java:
##########
@@ -700,7 +719,7 @@ public void dropPartitionsCache(ExternalTable dorisTable,
List<String> partition
partitionValuesMap.remove(partitionId);
if (invalidPartitionCache) {
- invalidatePartitionCache(dorisTable, partitionName);
+ invalidatePartitionCache(nameMapping, partitionName);
Review Comment:
[P1] Run drop invalidation even when partition values are not cached
This changed call is still unreachable when
`partitionValuesCache.getIfPresent(key)` returns null, because
`dropPartitionsCache` returns at lines 704-706 before entering the loop. That
is one of the independent-eviction states this PR is meant to handle: an HMS
drop event can leave the partition/file entries alive, and recreating the same
partition at the same warehouse location lets a later scan reload the name list
but reuse the stale file listing. Please invoke
`invalidatePartitionCache(nameMapping, partitionName)` for every dropped name
when `invalidPartitionCache` is true even if the partition-values entry is
absent, while keeping the copy/update work conditional on that entry, and add a
caller-level regression test.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/hive/HiveMetaStoreCache.java:
##########
@@ -529,21 +529,40 @@ public void invalidateTableCache(NameMapping nameMapping)
{
}
public void invalidatePartitionCache(ExternalTable dorisTable, String
partitionName) {
- NameMapping nameMapping = dorisTable.getOrBuildNameMapping();
+ invalidatePartitionCache(dorisTable.getOrBuildNameMapping(),
partitionName);
+ }
+
+ public void invalidatePartitionCache(NameMapping nameMapping, String
partitionName) {
long id = Util.genIdByName(nameMapping.getLocalDbName(),
nameMapping.getLocalTblName());
- PartitionValueCacheKey key = new PartitionValueCacheKey(nameMapping,
null);
- HivePartitionValues partitionValues =
partitionValuesCache.getIfPresent(key);
- if (partitionValues != null) {
- Long partitionId =
partitionValues.partitionNameToIdMap.get(partitionName);
- List<String> values =
partitionValues.partitionValuesMap.get(partitionId);
- PartitionCacheKey partKey = new PartitionCacheKey(nameMapping,
values);
- HivePartition partition = partitionCache.getIfPresent(partKey);
- if (partition != null) {
- fileCacheRef.get().invalidate(new FileCacheKey(id,
partition.getPath(),
- null, partition.getPartitionValues()));
- partitionCache.invalidate(partKey);
- }
+ // Derive the partition values directly from the partition name (the
partition-values cache is
+ // populated the same way, via HiveUtil.toPartitionValues) instead of
reading them from that
+ // cache, so file cache invalidation still runs when the table's
partition-values cache entry has
+ // been evicted while its file listings are still cached.
+ List<String> values = HiveUtil.toPartitionValues(partitionName);
+
+ PartitionCacheKey partKey = new PartitionCacheKey(nameMapping, values);
+ HivePartition partition = partitionCache.getIfPresent(partKey);
+ LoadingCache<FileCacheKey, FileCacheValue> fileCache =
fileCacheRef.get();
+ if (partition == null) {
+ // Partition metadata cache miss: the exact FileCacheKey cannot be
rebuilt here because it
+ // needs the partition path and input format carried by
HivePartition. Invalidate this
+ // table's cached file listings for the partition by (table id +
partition values). Scoping
+ // by table id is intentional: matching partition values alone
would also drop other tables'
+ // listings that merely share the same partition value names (e.g.
dt=...) at a different
+ // location, forcing needless re-listing. The exact-key path below
(taken when the partition
+ // is cached) already clears a listing regardless of which table
id populated it.
+ fileCache.asMap().keySet().forEach(k -> {
+ if (k.isSameTable(id) &&
Objects.equals(k.getPartitionValues(), values)) {
Review Comment:
[P1] Do not gate shared file-cache entries by a non-key owner id
`FileCacheKey.equals/hashCode` exclude `id` and identify normal entries by
location and partition values. Thus if table A first caches a partition and
table B points at the same location/values, B reuses A's entry, whose stored
key still carries A's id. After B's `partitionCache` entry is evicted (the
state this branch is meant to repair), refreshing B fails this `isSameTable(B)`
check and leaves the shared stale listing that B will hit again. The added
other-table test uses a different location, so it cannot exercise this
collision. Please make cache identity and invalidation ownership consistent (or
conservatively invalidate shared matching entries) and cover the same-location,
other-table-first sequence.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
