github-actions[bot] commented on code in PR #65126:
URL: https://github.com/apache/doris/pull/65126#discussion_r3636028980
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalDatabase.java:
##########
@@ -442,27 +471,17 @@ public DatabaseProperty getDbProperties() {
@Override
public boolean isTableExist(String tableName) {
SessionContext sessionContext = SessionContext.current();
+ if (extCatalog.shouldBypassTableNameCache(sessionContext)) {
Review Comment:
**[P2] Keep direct session table-existence checks as point lookups**
`shouldBypassTableNameCache()` is true for delegated Iceberg REST sessions,
so moving this guard outside the case-insensitive/name-mapping branch makes
every ordinary `isTableExist()` enumerate the entire database through
`findTableNamePairWithoutCache()` before it still calls `tableExists()`.
`IcebergMetadataOps.listTableNames()` issues `listTables()` and may also list
views, whereas the pre-change mode-0 path sent only the point existence
request. On large REST catalogs, each existence check therefore becomes
O(number of tables) work and extra network round trips. Please enumerate only
when a local-to-remote name transformation is actually required; otherwise call
`tableExist()` directly with the requested name.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/metacache/MetaCacheEntry.java:
##########
@@ -141,36 +200,75 @@ public V getIfPresent(K key) {
return data.getIfPresent(key);
}
+ @Nullable
+ public V findIfPresent(Predicate<K> keyPredicate) {
+ if (!effectiveEnabled) {
+ return null;
+ }
+ // Replay-only fallback needs a cache-only scan over current hot keys
without triggering load-through.
+ for (java.util.Map.Entry<K, V> entry : data.asMap().entrySet()) {
+ if (keyPredicate.test(entry.getKey())) {
+ return entry.getValue();
+ }
+ }
+ return null;
+ }
+
public void put(K key, V value) {
+ // Public mutations participate in generation control so in-flight
loads cannot overwrite them later.
+ Objects.requireNonNull(key, "key can not be null");
+ Objects.requireNonNull(value, "value can not be null");
if (!effectiveEnabled) {
return;
}
- data.put(key, value);
+ synchronized (publishLock(key)) {
+ bumpGeneration(key);
+ beforePublicMutationWriteForTest(key);
+ data.put(key, value);
+ }
+ }
+
+ public V compute(K key, BiFunction<K, V, V> remappingFunction) {
+ // Public compute must also advance the stripe generation before
mutating the cache state.
+ Objects.requireNonNull(key, "key can not be null");
+ Objects.requireNonNull(remappingFunction, "remappingFunction can not
be null");
+ if (!effectiveEnabled) {
+ return null;
+ }
+ synchronized (publishLock(key)) {
+ bumpGeneration(key);
+ beforePublicMutationWriteForTest(key);
+ return data.asMap().compute(key, remappingFunction);
+ }
}
public void invalidateKey(K key) {
- invalidateGeneration.incrementAndGet();
- if (data.asMap().remove(key) != null) {
- invalidateCount.incrementAndGet();
+ Objects.requireNonNull(key, "key can not be null");
+ synchronized (publishLock(key)) {
+ bumpGeneration(key);
+ if (data.asMap().remove(key) != null) {
+ invalidateCount.incrementAndGet();
+ }
}
}
public void invalidateIf(Predicate<K> predicate) {
- invalidateGeneration.incrementAndGet();
- data.asMap().keySet().removeIf(key -> {
+ Objects.requireNonNull(predicate, "predicate can not be null");
+ // Cover in-flight manual loads whose keys are still outside the cache
map.
+ bumpAllGenerations();
Review Comment:
**[P2] Do not fence unrelated loads during selective invalidation**
`invalidateIf()` is used for narrow table/database invalidations, but this
unconditional `bumpAllGenerations()` advances every stripe before the predicate
is evaluated. Consequently, an in-flight schema/partition/file load for
unrelated table B that overlaps an event for table A fails the generation check
and is returned without being cached; repeated targeted events can keep
unrelated entries from warming and force repeated remote reads across the
catalog. Please fence only matching cached or in-flight keys (and add an
A-invalidation/B-load test), while retaining the global bump for
`invalidateAll()`.
--
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]
