github-actions[bot] commented on code in PR #64160:
URL: https://github.com/apache/doris/pull/64160#discussion_r3757688127
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/plugin/PluginDrivenExternalCatalog.java:
##########
@@ -1306,19 +1340,15 @@ private static ConnectorColumnPath
toConnectorPath(ColumnPath columnPath) {
/**
* Replays the base {@link ExternalCatalog} per-op bookkeeping for a
connector-driven schema change.
*
- * <p>The base column ops only emit the editlog ({@code
logRefreshExternalTable}); the actual cache
- * invalidation is delegated INTO {@code metadataOps.refreshTable ->
RefreshManager.refreshTableInternal}.
- * Since PluginDrivenExternalCatalog has no {@code metadataOps}, this
helper does BOTH explicitly: the
- * {@code createForRefreshTable} editlog (LOCAL names, replay-neutral) and
a {@code refreshTableInternal}
- * (re-resolving the local cached table by its REMOTE names, mirroring
legacy
- * {@code IcebergMetadataOps.refreshTable}). {@code refreshTableInternal}
is the single source of truth for
- * the cache work ({@code unsetObjectCreated} + {@code setUpdateTime} +
{@code invalidateTableCache} + the
- * connector-side per-table cache drop), so it must NOT be re-inlined here.
+ * <p>The base column ops emit the edit log and delegate cache work to
+ * {@code RefreshManager.refreshTableInternal}. PluginDriven has no {@code
metadataOps}, so this helper does
+ * both explicitly. It re-resolves the shared cached objects because
session-scoped metadata bypass can return
+ * a transient database/table object that must not be used for
shared-cache bookkeeping.</p>
*/
protected void afterExternalDdl(ExternalTable externalTable, long
updateTime) {
Env.getCurrentEnv().getEditLog().logRefreshExternalTable(
- ExternalObjectLog.createForRefreshTable(getId(),
- externalTable.getDbName(), externalTable.getName(),
updateTime));
+ ExternalObjectLog.createForRefreshTable(getId(),
externalTable.getDbId(),
+ externalTable.getDbName(), externalTable.getId(),
externalTable.getName(), updateTime));
getDbForReplay(externalTable.getRemoteDbName()).ifPresent(db ->
Review Comment:
**[P1] Use local identities here and retain an exact cold-cache fallback**
The edit log above correctly stores local names/IDs, but this lookup passes
`getRemoteDbName()`/`getRemoteName()` into replay caches keyed by local names.
Under the supported arbitrary mode-0 mapping, for example `RemoteDB ->
LocalDB`, mode 0 does not reverse-map the remote lookup, so even a hot shared
object is missed. A delegated Iceberg REST session can also return usable
transient DB/table objects without publishing the shared cache entries, making
the same optional chain empty even when names match.
The remote DDL has succeeded and been journaled, but the leader does not
replay its own log; an empty lookup therefore skips connector, engine,
row-count, and sorted-partition cleanup on the coordinator. Please look up the
shared objects with `externalTable.getDbName()`/`getName()`, and when they are
absent use the already-known remote names plus exact local IDs/names to perform
connector -> metadata -> row-count invalidation directly. Add production-shaped
arbitrary-mapping and delegated-session tests instead of a lookup-agnostic
`getDbForReplay` stub.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalMetaCacheMgr.java:
##########
@@ -208,26 +211,46 @@ public void removeCatalogByEngine(long catalogId, String
engine) {
() -> cache.invalidateCatalog(catalogId)));
}
- public void invalidateDb(long catalogId, String dbName) {
+ public void invalidateDbMetadataCache(long catalogId, String dbName) {
routeCatalogEngines(catalogId, cache -> safeInvalidate(
- cache, catalogId, "invalidateDb", () ->
cache.invalidateDb(catalogId, dbName)));
+ cache, catalogId, "invalidateDbMetadataCache", () ->
cache.invalidateDb(catalogId, dbName)));
// Cache B has no db-scoped eviction key, so a db-level REFRESH drops
ALL entries (coarse but
- // correct -- a rebuild is cheap and lazy). Mirrors invalidateTable's
Cache B wiring.
+ // correct -- a rebuild is cheap and lazy). Mirrors
invalidateTableMetadataCache's Cache B wiring.
invalidateSortedPartitionsCache();
}
- public void invalidateTable(long catalogId, String dbName, String
tableName) {
+ public void invalidateDb(long catalogId, long dbId, String dbName) {
+ invalidateDbMetadataCache(catalogId, dbName);
+ rowCountCache.invalidateDb(catalogId, dbId);
+ }
+
+ public void invalidateDb(ExternalDatabase<?> dorisDb) {
+ invalidateDb(dorisDb.getCatalog().getId(), dorisDb.getId(),
dorisDb.getFullName());
+ }
+
+ public void invalidateTableMetadataCache(long catalogId, String dbName,
String tableName) {
routeCatalogEngines(catalogId, cache -> safeInvalidate(
- cache, catalogId, "invalidateTable",
+ cache, catalogId, "invalidateTableMetadataCache",
() -> cache.invalidateTable(catalogId, dbName, tableName)));
// Also drop the Nereids sorted-partition-ranges cache for this
external table so binary-search
// pruning does not serve ranges older than the refreshed metadata.
- CatalogIf<?> ctl = getCatalog(catalogId);
- if (ctl != null) {
-
Env.getCurrentEnv().getSortedPartitionsCacheManager().invalidateTable(ctl.getName(),
dbName, tableName);
+ CatalogIf<?> catalog = getCatalog(catalogId);
+ if (catalog != null) {
+ Env.getCurrentEnv().getSortedPartitionsCacheManager()
Review Comment:
**[P1] Fence Cache B publication and validate the actual partition items**
This point invalidation is not a publication fence. A query can pin the old
MVCC partition metadata, a normal refresh can invalidate the connector and this
Cache B entry, and then the older query can enter
`NereidsSortedPartitionsCacheManager.loadCache()` and unconditionally `put`
ranges built from its retained pin. A later query uses the same local name key
and deterministic table ID;
`PluginDrivenMvccExternalTable.getPartitionMetaVersion()` compares only the
partition-name set, so identical names with changed `PartitionItem`
bounds/type/spec accept the stale ranges and can prune incorrectly.
An extra branch-local invalidation is insufficient because the stale put can
occur after it. Cache validity must include the exact immutable
name-to-`PartitionItem` content (or an equivalent snapshot/version), or use a
generation/admission protocol that rejects publication from a pin older than
the invalidation. Apply the same exact local-key fence to coordinator/follower
CREATE, DROP, and RENAME even when shared objects are absent, with CREATE as
the final same-name-reuse barrier. Please add deterministic
identical-name/different-item and invalidation-between-pin-and-put tests, plus
the retained-pin DROP/refill/CREATE sequence.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalDatabase.java:
##########
@@ -125,7 +125,7 @@ public void resetMetaToUninitialized() {
this.initialized = false;
invalidateAllTableCache();
}
-
Env.getCurrentEnv().getExtMetaCacheMgr().invalidateDb(extCatalog.getId(),
getFullName());
+ Env.getCurrentEnv().getExtMetaCacheMgr().invalidateDb(this);
Review Comment:
**[P1] Avoid a global row-count scan for every removed DB object**
`ExternalCatalog.databases` has a synchronous removal listener that calls
`resetMetaToUninitialized()` for explicit removals, replacements, TTL/capacity
eviction, and `invalidateAll()`. This new call therefore expands every removed
database into `ExternalRowCountCache.invalidateDb()`, which scans the entire
global row-count cache while holding its single publication write lock. A
catalog refresh first removes every hot DB object, so it now performs one full
scan per hot database and then another catalog-wide scan; with the configured
maxima this can revisit roughly 100 million entries. A hot single-DB unregister
also performs the listener invalidation and the explicit exact-ID invalidation.
The existing live thread only notes the cost of one requested O(N) scan; it
does not cover this new O(hot DBs * global entries) synchronous cascade or
passive-eviction cost. Please separate passive object teardown from structural
row-count invalidation and batch catalog cleanup to one catalog-scope eviction
(and one exact eviction for a single DB). A real-manager multi-DB test should
expose the repeated scans; the current mock-overload tests cannot.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/plugin/PluginDrivenExternalCatalog.java:
##########
@@ -860,22 +862,32 @@ public void truncateTable(String dbName, String
tableName, PartitionNamesInfo pa
// connector-bound handle is remote-resolved), mirroring base
ExternalCatalog.truncateTable.
Env.getCurrentEnv().getRefreshManager().refreshTableInternal(db,
dorisTable, updateTime);
Env.getCurrentEnv().getEditLog().logTruncateTable(
- new TruncateTableInfo(getName(), dbName, tableName,
partitions, updateTime));
+ new TruncateTableInfo(getName(), db.getId(), db.getFullName(),
dorisTable.getId(),
+ dorisTable.getName(), partitions, updateTime));
LOG.info("finished to truncate table {}.{}.{}", getName(), dbName,
tableName);
}
/**
- * Refreshes the local table cache on edit-log replay of a
connector-driven truncate. The base
- * {@link ExternalCatalog#replayTruncateTable} delegates to {@code
metadataOps.afterTruncateTable}, which is a
- * no-op for PluginDriven ({@code metadataOps == null}); this override
re-resolves the cached table by the
- * replayed LOCAL names and runs {@code refreshTableInternal} (the same
effect the master path applied),
- * mirroring legacy {@code HiveMetadataOps.afterTruncateTable}.
+ * Replays cache invalidation for a connector-driven truncate without
loading remote metadata. A cached table
+ * follows the normal refresh path. Otherwise the edit log's exact IDs
invalidate engine and row-count caches,
+ * while connector invalidation widens to the database or catalog scope
when the remote table name is unavailable.
*/
@Override
public void replayTruncateTable(TruncateTableInfo info) {
- getDbForReplay(info.getDb()).ifPresent(db ->
- db.getTableForReplay(info.getTable()).ifPresent(tbl ->
-
Env.getCurrentEnv().getRefreshManager().refreshTableInternal(db, tbl,
info.getUpdateTime())));
+ Optional<ExternalDatabase<? extends ExternalTable>> db =
getDbForReplay(info.getDb());
+ Optional<? extends ExternalTable> table = db.flatMap(database ->
database.getTableForReplay(info.getTable()));
+ if (table.isPresent()) {
+ Env.getCurrentEnv().getRefreshManager()
+ .refreshTableInternal(db.get(), table.get(),
info.getUpdateTime());
+ return;
+ }
+ if (db.isPresent()) {
+ getConnector().invalidateDb(db.get().getRemoteName());
+ } else {
+ invalidateAllConnectorCachesIfPresent();
+ }
+ Env.getCurrentEnv().getExtMetaCacheMgr().invalidateTable(
Review Comment:
**[P1] Handle legacy truncate records before using the exact-ID fallback**
This fallback is correct only for records written by the new constructor.
Before this change, external `TruncateTableInfo` persisted the
catalog/names/partitions/update time but did not populate `dbId` or `tblId`, so
an old leader's JSON gives a new follower zero for both fields. A hot DB can
retain its table-ID index and row-count entry after the table object is
passively evicted; replay then takes this cold-table branch. Metadata/Cache B
is cleared by name, but `invalidateTable(catalog, 0, ..., 0, ...)` misses the
real deterministic row-count key.
This is the old-record -> new-follower rolling-upgrade path, not the
current-record cold replay already covered by the existing truncate thread, and
it is a different persisted opcode from the existing legacy `ExternalObjectLog`
thread. Please recognize zero-ID name-bearing legacy truncate records and
derive the deterministic IDs, or conservatively invalidate the database/catalog
row-count scope. Add a test that deserializes the pre-change JSON shape and
replays it with a hot row count and evicted table object.
--
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]
