github-actions[bot] commented on code in PR #68196:
URL: https://github.com/apache/doris/pull/68196#discussion_r4130497976
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/hive/HMSExternalCatalog.java:
##########
@@ -320,21 +320,40 @@ public boolean registerDatabaseFromEvent(long dbId,
String dbName, long metadata
return false;
}
+ /** HMS notifications are unfiltered; excluded targets must not change
this catalog's caches. */
+ public boolean isPartitionEventTargetExcluded(String dbName, String
tableName) {
+ if (!isDatabaseAllowedByFilterIgnoringCase(dbName)) {
+ return true;
+ }
+ return getIncludeTableMap().entrySet().stream()
Review Comment:
[P2] Avoid rebuilding and logging the include list for each partition event.
All three new add/drop/alter-partition guards call this method; for each
processed event whose database passes the first filter, `getIncludeTableMap()`
splits and allocates the full configured map and logs it at INFO when
`include_table_list` is set, including events for visible tables. With many
entries and frequent notifications, parsing and full-map log writes scale with
their product and can delay HMS event catch-up. Keep a parsed filter snapshot
refreshed on property changes and remove or guard the per-event INFO dump.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalCatalog.java:
##########
@@ -1247,10 +1320,62 @@ public void unregisterDatabase(String dbName) {
if (LOG.isDebugEnabled()) {
LOG.debug("unregister database [{}]", dbName);
}
- if (isInitialized()) {
- metaCache.invalidate(dbName, Util.genIdByName(name, dbName));
+ if (!isInitialized()) {
+ Env.getCurrentEnv().getExtMetaCacheMgr().invalidateDb(getId(),
dbName);
+ return;
+ }
+ Optional<Pair<String, Long>> identity = getDbIdentityForDrop(dbName);
+ if (!identity.isPresent()) {
+ // A mode-2 remote-to-local mapping can disappear (for example
after a names refresh)
+ // while the resident database object survives. The canonical key
is then unknown, so
+ // treat the scope as unknown: retire every cached database object
and flush the engine
+ // caches and row counts catalog-wide instead of evicting the
wrong local key.
+ retireAllDatabaseObjectsWithoutEngineInvalidation();
Review Comment:
[P2] Keep HMS database events from flushing unrelated caches.
`DropDatabaseEvent` forwards lowercased names: for a visible mode-2 remote
`Foo`, `getDbIdentityForDrop("foo")` resolves `Foo` but looks only for a
historical `foo` ID, which the normal cache lacks, so this branch invalidates
the whole catalog instead of `Foo`. An excluded `archived` DROP or rename also
reaches this branch with no mapping and evicts hot sibling databases. The
previous route was name scoped. Distinguish current canonical, retained
historical, and filtered event identities so their cleanup stays narrow; test a
mixed-case visible DROP and an excluded DROP/rename beside an unrelated hot DB.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/hive/HMSExternalCatalog.java:
##########
@@ -320,21 +320,40 @@ public boolean registerDatabaseFromEvent(long dbId,
String dbName, long metadata
return false;
}
+ /** HMS notifications are unfiltered; excluded targets must not change
this catalog's caches. */
+ public boolean isPartitionEventTargetExcluded(String dbName, String
tableName) {
+ if (!isDatabaseAllowedByFilterIgnoringCase(dbName)) {
Review Comment:
[P2] Match HMS event filtering to the databases and tables Doris exposes.
`getFilteredDatabaseNames` applies exact-case DB filters, but this new guard
uses case-insensitive matching: with remote `sales` and
`exclude_database_list=SALES`, Doris exposes `sales` yet skips its
add/drop/alter partition events. Below, `listTableNames` looks up the exact
remote DB key, while this helper matches include-table keys ignoring case:
`include_table_list=SALES.cold` exposes `sales.hot` but skips its partition
events too. Both cases retain stale partition, file, and row-count metadata.
Resolve the canonical remote name and apply the same visibility rules as
listing; test both case-variant filters.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/CatalogMgr.java:
##########
@@ -931,15 +940,38 @@ private void
alterExternalCatalogPropsFenced(ExternalCatalog externalCatalog, Ca
Integer[] sec = {metadataRefreshIntervalSec,
metadataRefreshIntervalSec};
Env.getCurrentEnv().getRefreshManager().addToRefreshMap(catalogId,
sec);
}
- externalCatalog.modifyCatalogProps(newProps);
// The commit reset the catalog's execution context and closed its SDK
resources. Cached
// base generations and projections are bound to the replaced context;
retire them now so
// the next statement loads a generation the planning fences accept,
instead of retrying
- // against an unplannable cached generation until managed refresh.
+ // against an unplannable cached generation until managed refresh. The
properties are
+ // published before the reset's throwable cleanup, so retirement must
run either way.
Env currentEnv = Env.getCurrentEnv();
ExternalMetaCacheMgr cacheMgr = currentEnv == null ? null :
currentEnv.getExtMetaCacheMgr();
- if (cacheMgr != null) {
-
cacheMgr.onCatalogOperationalContextChanged(externalCatalog.getId());
+ try {
+ if (cacheMgr != null) {
+ // Close the old row-count generation before publishing the
new catalog context.
+ // The completion fence below also retires values loaded
during the reset.
+ cacheMgr.invalidateRowCountCache(externalCatalog.getId());
+ }
+ externalCatalog.modifyCatalogProps(newProps);
+ } catch (RuntimeException e) {
+ if (!isReplay) {
+ throw e;
+ }
+ // A follower must not terminate because a local connector cleanup
failed while applying
+ // an already-durable ALTER record. The property publication and
the derived-state
+ // transitions above are failure-safe, so the record is considered
applied.
+ LOG.warn("Failed to complete local cleanup while replaying ALTER
CATALOG for {}: {}",
+ externalCatalog.getName(), e.getMessage(), e);
+ } finally {
+ if (cacheMgr != null) {
+
cacheMgr.onCatalogOperationalContextChanged(externalCatalog.getId());
+ }
+ if (!isReplay && currentEnv != null) {
Review Comment:
[P1] Validate JDBC name mapping before journaling this ALTER. A JDBC `SET
PROPERTIES ("meta_names_mapping" = "not-json")` passes `checkProperties`, so
`modifyCatalogProps` publishes it before `JdbcIdentifierMapping` throws during
reset. This `finally` then journals the failed ALTER; replay suppresses the
mapping exception, leaving durable invalid properties and the old derived
mapping on leader and followers. The journal call was skipped for this failure
before this change. Validate the effective mapping before publication and add a
malformed-mapping ALTER/replay test.
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/RefreshManager.java:
##########
@@ -253,19 +289,30 @@ public void refreshExternalTableFromEvent(String
catalogName, String dbName, Str
if (!(catalog instanceof ExternalCatalog)) {
throw new DdlException("Only support refresh ExternalCatalog
Tables");
}
+ // Whole-table events are already committed remotely. Fence the row
count by cached identity
+ // before any database/table reload can fail and make the not-found
path return.
+ Env.getCurrentEnv().getExtMetaCacheMgr()
+ .invalidateRowCountCache(catalog.getId(), dbName, tableName);
DatabaseIf db = catalog.getDbNullable(dbName);
if (db == null) {
+ // Cold database: widen so independently resident engine entries
are retired too.
+ Env.getCurrentEnv().getExtMetaCacheMgr()
+ .invalidateTableByNameOrWider(catalog.getId(), dbName,
tableName);
Review Comment:
[P2] Skip excluded HMS targets on whole-table events. `InsertEvent` and
non-rename `AlterTableEvent` call this method for unfiltered notifications. In
mode 2, an excluded database has no canonical mapping, so this new miss
fallback invalidates the whole catalog; an excluded table in a visible database
widens to DB scope, clearing included siblings and their row counts for an
event Doris should ignore. The partition-event routes now check
`isPartitionEventTargetExcluded` before invalidating, but this route does not.
Apply the same guard here and cover excluded DB/table events with unrelated hot
entries.
--
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]
