github-actions[bot] commented on code in PR #69665:
URL: https://github.com/apache/doris/pull/69665#discussion_r4385790223
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/CatalogMgr.java:
##########
@@ -125,6 +125,8 @@ private RemovedCatalog removeCatalog(long catalogId) {
return null;
}
String catalogName = catalog.getName();
+ // Detach the old runtime before the mapping can be reused (not later
from a cleanup callback).
+ catalog.onClose();
Review Comment:
[P2] Unpublish the catalog before detaching its controller
Calling `onClose()` while the name/id mappings still expose this catalog
opens a new controller-publication race. After `detachAccessController()`
removes the old entry, a concurrent
`AccessControllerManager.getAccessControllerOrDefault()` can see the
still-current catalog, rebuild its controller, and complete every
`isCurrentCatalog()` check while DROP is retiring caches. Those lookups take
neither the CatalogMgr lock nor the catalog monitor and do not check `closed`.
DROP then removes the mappings without another detach, leaving the new
controller and its resources registered indefinitely. Rename has the same
old-name orphan window. Previously the mappings disappeared before `onClose()`,
so late publication failed its identity checks. Remove the mappings before
detachment while retaining the manager write lock, or fence controller
publication against the retiring generation.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalCatalog.java:
##########
@@ -988,37 +953,94 @@ public void setLastUpdateTime(long lastUpdateTime) {
@Override
public final void onClose() {
+ Runnable cleanup;
+ synchronized (this) {
Review Comment:
[P2] Avoid waiting for initialization under the global catalog lock
`removeCatalog()` now calls this method while holding CatalogMgr's write
lock, so acquiring this monitor can extend a remote initialization wait to
unrelated catalogs. For example, a query's first access to a `type=doris`
catalog holds the same monitor in `makeSureInitialized()` while
`RemoteDorisExternalCatalog.initLocalObjectsImpl()` performs the synchronous
HTTP `health()` request. A concurrent DROP/replayed DROP now holds the global
write lock until that request finishes, blocking unrelated CREATE/ALTER/DROP
and `SHOW CATALOGS`. Previously DROP removed the mappings and invoked
`onClose()` outside that lock, without this monitor acquisition. The
acknowledged same-catalog synchronous initialization limit does not require
holding the global lock during that wait; arrange monitor
acquisition/detachment outside the manager lock with identity revalidation
before committing removal.
--
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]
