github-actions[bot] commented on code in PR #69501:
URL: https://github.com/apache/doris/pull/69501#discussion_r4358895236


##########
fe/fe-core/src/main/java/org/apache/doris/datasource/plugin/PluginDrivenExternalTable.java:
##########
@@ -119,8 +119,24 @@ public PluginDrivenExternalTable(long id, String name, 
String remoteName,
      */
     public Optional<ConnectorTableHandle> resolveConnectorTableHandle(
             ConnectorSession session, ConnectorMetadata metadata) {
+        PluginDrivenExternalCatalog owner = (PluginDrivenExternalCatalog) 
catalog;
+        Connector connector = owner.getConnector();
+        ExternalDatabase<?> database = getDb();
+        Optional<Runnable> observation = database.observeTableLookup(this, () 
-> {
+            try {
+                connector.invalidateTable(db.getRemoteName(), getRemoteName());

Review Comment:
   [P2] Clear historical spellings during automatic missing-table repair
   
   With `lower_case_table_names=2`, warm `Foo`, externally replace it with 
`FOO`, and let a names-only refresh discover `FOO` (for example, a name-miss 
lookup for a newly created sibling). The old `Foo` object/schema and connector 
entries remain cached while `foo` resolves to `FOO`. If `FOO` is externally 
replaced by a new `Foo`, an ordinary query through `FOO` gets an empty handle 
and runs this callback, which invalidates only `FOO`; `observeTableLookup` also 
removes only that object/ID. The next names reload maps `foo` to `Foo` and can 
reuse the first incarnation's FE object/schema and Iceberg table/snapshot 
caches, serving stale metadata/data after automatic repair. Mode 1 can retain 
historical connector keys here too. Apply case-aware domain invalidation to 
automatic cleanup while preserving observation fencing, and cover the 
historical-spelling reuse sequence. This is a separate instance from the 
earlier explicit-REFRESH finding: this automatic callback never runs that 
now-broad
 ened refresh path.



##########
fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalDatabase.java:
##########
@@ -347,6 +350,44 @@ public Optional<T> getTableForReplay(long tableId) {
         return Optional.ofNullable(tables.getIfPresent(tableName));
     }
 
+    /**
+     * Captures only a shared base table with a matching retained names 
snapshot. Guard order is catalog DB,
+     * table names, then table object, matching incremental name/object 
publication. No remote load is performed.
+     */
+    public Optional<Runnable> observeTableLookup(ExternalTable table, Runnable 
invalidateMetadata) {
+        if (!initialized || 
extCatalog.shouldBypassSchemaCache(SessionContext.current())) {
+            return Optional.empty();
+        }
+        NameCacheValue names = getTableNamesValue(false);
+        if (names == null || 
!table.getRemoteName().equals(names.remoteNameOfLocalName(table.getName()))) {
+            return Optional.empty();
+        }
+        long metadataRefreshGeneration = table.getMetadataRefreshGeneration();
+        return extCatalog.observeDatabase(this).flatMap(database ->
+                tableNames.observe("", names).flatMap(membership ->
+                        tables.observe(table.getName(), table).map(object -> 
() -> MetaCacheCleanup.runDeferred(() -> {
+                            // Catalog reset/connector replacement holds this 
monitor too. Only local invalidation
+                            // runs here; remote reads happened before 
entering this guarded commit.
+                            synchronized (extCatalog) {
+                                if (!initialized || !extCatalog.isInitialized()
+                                        || 
table.getMetadataRefreshGeneration() != metadataRefreshGeneration
+                                        || 
Env.getCurrentEnv().getCatalogMgr().getCatalog(extCatalog.getId())
+                                                != extCatalog) {
+                                    return;
+                                }
+                                database.runIfCurrent(() -> 
membership.invalidateIfCurrent(() ->

Review Comment:
   [P2] Fence object loads until missing-table names are removed
   
   An `ExternalRowCountCache` worker can pass `getTableNullable(long)` 
initialization and capture the table name, then pause. Here the inner 
`object.invalidateIfCurrent` removes the object and releases its stripe/gates 
before the outer `membership.invalidateIfCurrent` finally invalidates names. If 
the worker resumes in that gap, it acquires a fresh table-publication lease and 
`buildTableForInit` accepts the table from the still-current names; its 
capability check can then block on the catalog monitor until this cleanup 
finishes. It subsequently publishes a replacement object and ID because the 
later names invalidation belongs to another cache owner and does not fence that 
lease. The dropped table is therefore cached/indexed again after 
confirmed-missing repair; subsequent observations skip cleanup once names no 
longer include it. Keep object publication fenced through retirement of the 
names snapshot, and add a deterministic numeric-lookup interleaving 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]

Reply via email to