github-actions[bot] commented on code in PR #64160:
URL: https://github.com/apache/doris/pull/64160#discussion_r3764824845
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalMetaCacheMgr.java:
##########
@@ -185,6 +186,7 @@ public void invalidateCatalog(long catalogId) {
// REFRESH must drop ALL of its entries -- else binary-search pruning
could serve ranges older than
// the refreshed metadata. Coarse but correct (a rebuild is cheap and
lazy). Mirrors invalidateTable.
invalidateSortedPartitionsCache();
+ rowCountCache.invalidateCatalog(catalogId);
Review Comment:
**[P1] Invalidate row counts when properties replace the source**
This catalog-scope row-count eviction is reached by `onRefreshCache(true)`
and catalog removal, but ordinary ALTER CATALOG property application calls
`resetToUninitialized(false, true)` (or `notifyPropertiesUpdated ->
resetToUninitialized(false)`). That closes and rebuilds the plugin connector
and clears the DB metadata caches, while `onRefreshCache(false)` skips this
method; only the special schema-cache-TTL property subsequently removes the
catalog cache. Changing an endpoint, warehouse, or other source-defining
property therefore rebuilds the same local catalog/DB/table IDs while retaining
a count fetched from the old source, so the first post-ALTER lookup can hit it
without consulting the new connector. Replay uses the same property-application
path. Treat every property reset that replaces the connector/source as a
catalog row-count barrier, ordered after source teardown and metadata cleanup,
and test a non-schema-cache source property on both coordinator and replay.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/plugin/PluginDrivenExternalCatalog.java:
##########
@@ -748,8 +748,9 @@ public void dropTable(String dbName, String tableName,
boolean isView, boolean i
// Uniform with the table branch: drop the connector's own caches
for this name (harmless no-op
// for a view, which carries no snapshot pin). Keyed by the REMOTE
names.
connector.invalidateTable(dorisTable.getRemoteDbName(),
dorisTable.getRemoteName());
- Env.getCurrentEnv().getEditLog().logDropTable(new
DropInfo(getName(), dbName, tableName));
- getDbForReplay(dbName).ifPresent(d ->
d.unregisterTable(tableName));
+ Env.getCurrentEnv().getEditLog().logDropTable(
+ new DropInfo(getName(), db.getFullName(),
dorisTable.getName()));
+ getDbForReplay(db.getFullName()).ifPresent(d ->
d.unregisterTable(dorisTable.getName()));
Review Comment:
**[P1] Do not gate DROP cleanup on a second DB-cache hit**
The method already retains exact `db` and `dorisTable` identities, but after
the remote DROP and connector invalidation it routes all local table/row-count
cleanup through this second cache-only lookup. The bounded top-level DB cache
can passively evict during the remote operation; its synchronous listener calls
`resetMetaToUninitialized()`, which now clears the table objects/ID index and
intentionally preserves row counts. This lookup then returns empty, so the
leader skips exact eviction even though it never replays its own journal. The
table branch at line 782 has the same race, and cold/uninitialized follower
`replayDropTable` similarly widens connector invalidation but lets the base
optional lookup skip local/row-count cleanup. A same-name recreation then
reuses the stale key. Make final cleanup independent of shared-cache residency
using the retained exact IDs (or deterministic logged identities); merely
calling the evicted DB's `unregisterTable` is insufficient because its
index was cleared. Add forced-eviction tests for table/view coordinator DROP
and cold replay.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/plugin/PluginDrivenExternalCatalog.java:
##########
@@ -819,7 +821,7 @@ public void renameTable(String dbName, String oldTableName,
String newTableName)
// local->remote mapping). Followers propagate this via
RefreshManager.replayRefreshTable's rename branch.
connector.invalidateTable(dorisTable.getRemoteDbName(),
dorisTable.getRemoteName());
connector.invalidateTable(dorisTable.getRemoteDbName(), newTableName);
- afterExternalRename(dbName, oldTableName, newTableName);
+ afterExternalRename(db.getFullName(), dorisTable.getName(),
newTableName);
Review Comment:
**[P1] Preserve the exact source identity across RENAME**
After the remote rename and both connector invalidations,
`afterExternalRename` discards the retained `db`/`dorisTable` identities and
gates old-name cleanup on another `getDbForReplay`. If the bounded DB cache
passively evicts in that interval, its synchronous listener clears table
objects/IDs but now deliberately preserves row counts; the helper skips
unregister/reset, and the leader does not replay its own rename journal. This
breaks the atomic-swap flow documented immediately above: with count 100 cached
for `t`, eviction during `RENAME t TO t_arch`, followed by `RENAME t_new TO t`,
lets the new table reuse deterministic key `t` and inherit 100. Thread the
retained exact IDs into final bookkeeping and evict the old row-count key
independently of cache residency (calling the reset DB's unregister alone is
insufficient after its index was cleared). Add an eviction hook between remote
rename and local bookkeeping and exercise the two-rename swap with distinct
counts.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/plugin/PluginDrivenExternalCatalog.java:
##########
@@ -551,7 +551,7 @@ public boolean createTable(CreateTableInfo createTableInfo)
throws UserException
// the table -- a table created out-of-band (another engine /
another FE) stays invisible to
// SHOW TABLES and to a subsequent SELECT until an unrelated
refresh. Every successful no-op
// must refresh the names, exactly like the created-here path.
- getDbForReplay(createTableInfo.getDbName()).ifPresent(d ->
d.resetMetaCacheNames());
+ getDbForReplay(db.getFullName()).ifPresent(d ->
d.resetMetaCacheNames());
Review Comment:
**[P1] Make CREATE/REGISTER the final row-count barrier**
This successful `IF NOT EXISTS` convergence only refreshes the table-name
cache. Plugin table/database IDs are deterministic from local names, so after
an out-of-band drop+recreate that this FE did not observe, the new incarnation
reuses the old `ExternalRowCountCache` key and the next `getRowCount()` returns
the previous object's count. The created-here arm, concurrent-create recovery,
`replayCreateTable`, and metastore `REGISTER_TABLE`/`REGISTER_DATABASE` paths
have the same omission: they invalidate connector/name state or publish the
object, but never make row-count eviction the final barrier. No contract
requires a REGISTER descriptor or this convergence path to be preceded by a
successfully applied UNREGISTER on this FE. This is independent of the existing
Cache B thread. Please evict the exact table key on every successful/no-op
table CREATE/REGISTER, use DB scope for database CREATE/REGISTER, mirror it in
replay, and add a preseeded same-name-reuse test.
--
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]
