924060929 commented on code in PR #69720:
URL: https://github.com/apache/doris/pull/69720#discussion_r4396985531
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/RefreshManager.java:
##########
@@ -162,17 +162,14 @@ public void handleRefreshTable(String catalogName, String
dbName, String tableNa
return;
}
- TableIf table = db.getTableNullable(tableName);
- if (table == null) {
- if (!ignoreIfNotExists) {
- throw new DdlException("Table " + tableName + " does not exist
in db " + dbName);
- }
- return;
- }
- long updateTime = System.currentTimeMillis();
- refreshTableInternal((ExternalDatabase) db, (ExternalTable) table,
updateTime, true);
+ // A forwarding FE may retain a dropped table that is cold on the
master. Refresh by name so
+ // remote absence cannot prevent invalidating that FE's stale lookup
through the edit log.
+ ExternalDatabase<?> externalDb = (ExternalDatabase<?>) db;
+ String localTableName = externalDb.getTableForReplay(tableName)
+ .map(ExternalTable::getName).orElse(tableName);
ExternalObjectLog log =
ExternalObjectLog.createForExplicitRefreshTable(
- catalog.getId(), db.getFullName(), table.getName(),
updateTime);
+ catalog.getId(), db.getFullName(), localTableName,
System.currentTimeMillis());
+ replayRefreshTable(log);
Review Comment:
Could we make the new invalidation contract explicit and share the
implementation through a private helper, instead of calling the replay entry
point from command execution?
The name-based fix is appropriate for the reported failure. A follower can
retain a dropped table while the master's table cache is cold.
`getTableNullable()` is a loading cache lookup, not an authoritative remote
existence check: a hot master can return the stale object, whereas a cold
master loads names and can return `null`.
`ExternalDatabase.buildTableForInit()` also converts some remote-access
failures into `null`. Making invalidation depend on that lookup therefore gives
the same refresh request different outcomes depending on cache state.
Changing `ignoreIfNotExists` to `true` alone would still return before
invalidation/journaling. Keeping the lookup and removing the early return would
handle a missing-table result, but retain an unnecessary synchronous
table-metadata dependency. It would also make a missing database silently
succeed. At this head, the only production caller and all six test calls pass
`false`, so there is no active two-mode API to preserve.
I suggest:
- Remove `ignoreIfNotExists` from this method and its callers; keep
missing-catalog/type/database validation and the existing command privilege
check.
- Keep the cache-only table-name resolution added by this PR. A missing
table object must not prevent recording an invalidation for other FEs.
- Extract the ordinary refresh application from `replayRefreshTable()` into
a private helper. Command execution owns validation and journaling; replay owns
journal checks and rename dispatch; both apply exactly the same cache effects.
- Keep the journal's **`invalidateTableLookup`** flag: it is different from
`ignoreIfNotExists` and is still needed to distinguish explicit lookup
invalidation from content-only refresh.
The explicit contract would be: **refreshing a table name in an existing
external database invalidates metadata even if the remote table no longer
exists; success does not assert remote existence or preload its schema.** This
intentionally also permits a never-existing/misspelled table name, so please
document that user-visible behavior in the REFRESH documentation as well as the
release note.
Complete proposed replacements for `handleRefreshTable()` and
`replayRefreshTable()`, plus the new helper:
```java
/**
* Invalidates metadata for a table name in an existing external database.
* The remote table does not have to exist; a cold master may need to
invalidate
* stale lookups retained by other FEs.
*/
public void handleRefreshTable(String catalogName, String dbName, String
tableName)
throws DdlException {
Env env = Env.getCurrentEnv();
CatalogIf catalog = catalogName != null ?
env.getCatalogMgr().getCatalog(catalogName) : env.getCurrentCatalog();
if (catalog == null) {
throw new DdlException("Catalog " + catalogName + " doesn't exist.");
}
if (!(catalog instanceof ExternalCatalog)) {
throw new DdlException("Only support refresh ExternalCatalog
Tables");
}
ExternalCatalog externalCatalog = (ExternalCatalog) catalog;
ExternalDatabase<?> db = externalCatalog.getDbNullable(dbName);
if (db == null) {
throw new DdlException("Database " + dbName + " does not exist in
catalog " + catalog.getName());
}
// Retain the canonical local identity when cached, without loading a
remote table.
String localTableName = db.getTableForReplay(tableName)
.map(ExternalTable::getName)
.orElse(tableName);
long updateTime = System.currentTimeMillis();
ExternalObjectLog log = ExternalObjectLog.createForExplicitRefreshTable(
catalog.getId(), db.getFullName(), localTableName, updateTime);
applyTableRefresh(externalCatalog, db.getFullName(), localTableName,
updateTime, true);
env.getEditLog().logRefreshExternalTable(log);
}
public void replayRefreshTable(ExternalObjectLog log) {
ExternalCatalog catalog = (ExternalCatalog)
Env.getCurrentEnv().getCatalogMgr().getCatalog(log.getCatalogId());
if (catalog == null) {
LOG.warn("failed to find catalog when replaying refresh table: {}",
log.debugForRefreshTable());
return;
}
String localDbName = log.getDbName();
String localTableName = log.getTableName();
if (Strings.isNullOrEmpty(localDbName) ||
Strings.isNullOrEmpty(localTableName)) {
LOG.warn("refresh table replay log has no local name: {}",
log.debugForRefreshTable());
return;
}
if (!Strings.isNullOrEmpty(log.getNewTableName())) {
replayRenameTable(log, catalog, catalog.getDbForReplay(localDbName));
return;
}
applyTableRefresh(catalog, localDbName, localTableName,
log.getLastUpdateTime(), log.isInvalidateTableLookup());
}
/**
* Applies refresh effects without synchronously loading remote table
metadata.
* Shared by command execution and journal replay; does not write an edit
log.
*/
private void applyTableRefresh(ExternalCatalog catalog, String localDbName,
String localTableName,
long updateTime, boolean invalidateTableLookup) {
long catalogId = catalog.getId();
long dbId = Util.genIdByName(catalog.getName(), localDbName);
long tableId = Util.genIdByName(catalog.getName(), localDbName,
localTableName);
boolean invalidateDatabase = invalidateTableLookup &&
catalog.getLowerCaseTableNames() != 0;
ExternalMetaCacheMgr cacheMgr = Env.getCurrentEnv().getExtMetaCacheMgr();
Optional<ExternalDatabase<? extends ExternalTable>> db =
catalog.getDbForReplay(localDbName);
if (!db.isPresent()) {
try {
// Remote name mappings are unavailable without a retained
database object.
invalidateAllConnectorCachesIfPresent(catalog);
} finally {
if (invalidateDatabase) {
cacheMgr.invalidateDb(catalogId, dbId, localDbName);
} else {
cacheMgr.invalidateTable(catalogId, dbId, localDbName,
tableId, localTableName);
}
}
LOG.info("database object cache is cold when refreshing table; "
+ "invalidated caches by local names {}.{} in
catalog {}",
localDbName, localTableName, catalogId);
return;
}
ExternalDatabase<? extends ExternalTable> database = db.get();
Optional<? extends ExternalTable> table =
database.getTableForReplay(localTableName);
if (table.isPresent()) {
// Preserve generation fencing, object-created state and metadata
update time.
refreshTableInternal(database, table.get(), updateTime,
invalidateTableLookup);
return;
}
try {
if (catalog instanceof PluginDrivenExternalCatalog) {
// A cold table object cannot provide its mapped remote name.
((PluginDrivenExternalCatalog)
catalog).getConnector().invalidateDb(database.getRemoteName());
}
} finally {
try {
if (invalidateTableLookup) {
database.invalidateTableLookup(localTableName);
}
} finally {
// Case-insensitive lookup may refer to a different retained
spelling/table ID.
if (invalidateDatabase) {
cacheMgr.invalidateDb(catalogId, dbId,
database.getFullName());
} else {
cacheMgr.invalidateTable(
catalogId, dbId, database.getFullName(), tableId,
localTableName);
}
}
}
LOG.info("table object cache is cold when refreshing table; "
+ "invalidated caches by local name {}.{} in catalog {}",
database.getFullName(), localTableName, catalogId);
}
```
Keep the existing `refreshTableAfterExternalMutation()`,
`refreshTableInternal()`, `replayRenameTable()`, and
`invalidateAllConnectorCachesIfPresent()` implementations. No new imports are
required. The helper above retains the existing hot/cold paths,
connector-before-engine/row-count invalidation order, case-insensitive
database-wide fallback, and nested `finally` cleanup. Hot objects still use
`refreshTableInternal()` for generation fencing and update time. The explicit
command still applies locally before writing its existing journal; this
proposal does not change journal format or ordering.
Update the production call in `RefreshTableCommand.run()` to:
```java
Env.getCurrentEnv().getRefreshManager()
.handleRefreshTable(tableNameInfo.getCtl(), tableNameInfo.getDb(),
tableNameInfo.getTbl());
```
Also remove the trailing `, false` from all six direct test calls:
- `RefreshManagerTest`: `testExplicitRefreshReloadsTableNamesAndObject` and
`testExplicitRefreshOnColdMasterInvalidatesDroppedTableOnFollower`.
- `RefreshTableTest`: `testRefreshTable`.
- `PluginDrivenMissingTableTest`: `verifyDelayedMissing`,
`numericLookupCapturedBeforeRefreshRepublishesItsIdentity`, and
`foldedNamesRefreshClearsHistoricalRemoteSpellings`.
The existing cold-master/warm-follower, repeat-refresh and recreation
coverage should remain. I would additionally cover a missing database (still an
error, with no refresh journal), a resolvable database with a cold table cache
whose remote table-name loader fails if called (refresh must not invoke it),
and actual mixed-case names across master/follower in modes 1/2.
This is a non-blocking interface/structure recommendation, not a
demonstrated wrong-result or crash finding. The proposed replacement code has
been checked against the current source at `e77bfeab48b5`, but has **not** been
compiled or tested; the PR's existing validation results do not validate this
proposed refactor.
--
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]
