github-actions[bot] commented on code in PR #68238:
URL: https://github.com/apache/doris/pull/68238#discussion_r4059097536
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalDatabase.java:
##########
@@ -128,7 +128,7 @@ public void resetMetaToUninitialized() {
metaCache.invalidateAll();
}
}
-
Env.getCurrentEnv().getExtMetaCacheMgr().invalidateDb(extCatalog.getId(),
getFullName());
+ Env.getCurrentEnv().getExtMetaCacheMgr().invalidateDb(this);
Review Comment:
[P2] Avoid rescanning the entire Paimon SDK table cache once per cached
database during catalog refresh. `ExternalCatalog.refreshMetaCacheOnly()`
invalidates the legacy database cache synchronously, whose removal listener
invokes this new typed path for every cached DB; each Paimon call snapshots and
walks the full SDK key set, and `invalidCache=true` then performs the
catalog-wide scan that could clear it once. Scheduled refresh always uses that
mode, so the work can grow as cached databases x SDK keys while repeatedly
taking the SDK write fence. Suppress per-DB SDK invalidation during
catalog-wide retirement or maintain a DB-to-key index, and add a multi-DB
refresh regression.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonExternalCatalog.java:
##########
@@ -125,11 +132,68 @@ public Table getPaimonTable(NameMapping nameMapping) {
}
public Table getPaimonTable(NameMapping nameMapping, String branch, String
queryType) {
- makeSureInitialized();
Identifier identifier = tableIdentifier(nameMapping, branch,
queryType);
return loadPaimonTable(nameMapping, queryType, identifier);
}
+ public synchronized void invalidatePaimonTable(NameMapping nameMapping)
throws Exception {
+ // Property changes reset and close the SDK catalog before retiring
Doris cache entries.
+ // Do not recreate that catalog merely to invalidate an already
retired generation.
+ if (!isInitialized()) {
+ return;
+ }
+ Identifier identifier = tableIdentifier(nameMapping, null, null);
+ withSdkCatalogCacheWriteLock(() -> executionAuthenticator.execute(()
-> {
+ catalog.invalidateTable(identifier);
Review Comment:
[P1] Invalidate case-equivalent SDK keys for case-insensitive catalogs.
`sdkTableExists` can cache the statement's exact spelling (for example
`db.FOO`) even when Hive resolves it to the existing `db.foo`; Paimon
`Identifier` equality and `CachingCatalog.invalidateTable`'s branch scan
compare strings exactly. This resolved-object path later invalidates only the
canonical `NameMapping` spelling, so REFRESH TABLE or DROP TABLE can leave the
alternate key and a subsequent point lookup reuses the stale handle. Match
cached identifiers according to `catalog.caseSensitive()` (or canonicalize
before publication), and cover alternate-case warmups for both refresh and drop.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonExternalCatalog.java:
##########
@@ -125,11 +132,68 @@ public Table getPaimonTable(NameMapping nameMapping) {
}
public Table getPaimonTable(NameMapping nameMapping, String branch, String
queryType) {
- makeSureInitialized();
Identifier identifier = tableIdentifier(nameMapping, branch,
queryType);
return loadPaimonTable(nameMapping, queryType, identifier);
}
+ public synchronized void invalidatePaimonTable(NameMapping nameMapping)
throws Exception {
+ // Property changes reset and close the SDK catalog before retiring
Doris cache entries.
+ // Do not recreate that catalog merely to invalidate an already
retired generation.
+ if (!isInitialized()) {
+ return;
+ }
+ Identifier identifier = tableIdentifier(nameMapping, null, null);
+ withSdkCatalogCacheWriteLock(() -> executionAuthenticator.execute(()
-> {
+ catalog.invalidateTable(identifier);
+ return null;
+ }));
+ }
+
+ public synchronized void invalidatePaimonDatabase(String remoteDbName)
throws Exception {
+ if (!isInitialized()) {
+ return;
+ }
+ withSdkCatalogCacheWriteLock(() -> {
+ invalidateCachedPaimonTables(identifier ->
identifier.getDatabaseName().equals(remoteDbName));
+ return null;
+ });
+ }
+
+ public synchronized void invalidatePaimonCatalog() throws Exception {
+ if (!isInitialized()) {
+ return;
+ }
+ withSdkCatalogCacheWriteLock(() -> {
+ invalidateCachedPaimonTables(ignored -> true);
+ return null;
+ });
+ }
+
+ private void invalidateCachedPaimonTables(Predicate<Identifier> predicate)
throws Exception {
+ // A property ALTER closes the old SDK catalog before Doris retires
its cache entries.
+ // The new SDK catalog must remain lazily initialized in that callback.
+ if (!isInitialized() || !(catalog instanceof CachingCatalog)) {
Review Comment:
[P1] Reach the inner CachingCatalog when privilege checking decorates it.
Paimon 1.4.2 builds `PrivilegedCatalog(CachingCatalog(delegate))` when
warehouse privileges are enabled, so this type check returns without touching
the real cache. TABLE invalidation/reload is broken on the same wrapper:
`DelegateCatalog` does not forward `invalidateTable`, and `Catalog`'s default
implementation is a no-op, while `getTable` does forward into the cached
catalog. Consequently REFRESH TABLE/DATABASE/CATALOG can immediately reuse the
pre-refresh handle for every privilege-enabled catalog. Traverse the public
`DelegateCatalog.wrapped()` chain (or retain the actual `CachingCatalog`) for
invalidation, and add wrapper regressions for all three scopes.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonExternalCatalog.java:
##########
@@ -140,13 +204,12 @@ public Table getPaimonTable(NameMapping nameMapping,
String branch, String query
* Doris REFRESH can repopulate its cache with the same stale Paimon table
handle.
*/
public Table reloadPaimonTable(NameMapping nameMapping) {
- makeSureInitialized();
Identifier identifier = tableIdentifier(nameMapping, null, null);
try {
- return executionAuthenticator.execute(() -> {
+ return withInitializedSdkCatalogCacheWriteLock(() ->
executionAuthenticator.execute(() -> {
Review Comment:
[P2] Avoid holding the catalog-wide exclusive lock across every remote table
miss. Every Doris `tableEntry` miss reaches this method, so one cold/expired
table now owns the single fair write lock for `invalidateTable` plus the full
remote `getTable`; unrelated misses serialize and all TVF/existence/schema
reads queue behind that network latency. Preserve the global fence for
catalog/database invalidation and close, but use a per-identifier lock or
generation for the same-key reload race, with a two-table blocked-delegate
regression.
--
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]
