github-actions[bot] commented on code in PR #66269:
URL: https://github.com/apache/doris/pull/66269#discussion_r3710591580
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/authorizer/ranger/hive/RangerHiveAccessController.java:
##########
@@ -198,6 +258,28 @@ public boolean checkWorkloadGroupPriv(UserIdentity
currentUser, String workloadG
return true;
}
+ @Override
+ public List<? extends RowFilterPolicy> evalRowFilterPolicies(UserIdentity
currentUser, String ctl, String db,
+ String tbl) {
+ lifecycleLock.readLock().lock();
+ try {
+ return closed ? new ArrayList<>() :
super.evalRowFilterPolicies(currentUser, ctl, db, tbl);
Review Comment:
**[P1] Fail closed when teardown wins before policy evaluation**
`getAccessControllerOrDefault()` returns the controller before this
lifecycle read lock is acquired, so ALTER/DROP can detach and close that object
in between. The query then reaches this new branch and receives the same empty
list/`Optional.empty()` used for a genuine absence of Ranger policy;
`LogicalCheckPolicy` consequently keeps raw slots and adds no row filter, while
a later SELECT check can succeed against the replacement/default controller.
The existing lifecycle thread reported an NPE/use-after-cleanup; this is the
distinct fail-open outcome introduced by the new guards. Keep a
generation/lifecycle lease across controller selection and the complete policy
call, or abort/retry planning for a stale controller, and add latch tests for
both row filters and masks.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalCatalog.java:
##########
@@ -849,8 +884,29 @@ public void setLastUpdateTime(long lastUpdateTime) {
}
@Override
- public void onClose() {
- removeAccessController();
+ public final void onClose() {
Review Comment:
**[P1] Retire a dropped catalog before releasing teardown**
A query can capture this `ExternalCatalog` before DROP removes it from
`CatalogMgr`, then resume after `onClose()` has finished. Because this method
neither marks the object retired nor fences the synchronized
`makeSureInitialized()`, a later `getDbNames()`/`getDbNullable()` on that
captured object can create a new plugin connector, context, and metadata caches
after the last registered cleanup; the object is no longer in either manager
map, so those resources have no future owner. This is the connector/context
analogue of the existing late-controller-publication thread, not the same map
issue. Establish retirement/generation under the catalog monitor before
detaching resources and make post-retirement initialization fail or retry;
cover the captured-uninitialized-catalog DROP interleaving with latches.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalCatalog.java:
##########
@@ -599,12 +599,28 @@ private List<Pair<String, String>>
getFilteredDatabaseNames() {
* and reloaded during the refresh process.
*/
public void resetToUninitialized(boolean invalidCache) {
+ resetToUninitialized(invalidCache, false);
+ }
+
+ private Runnable resetToUninitialized(boolean invalidCache, boolean
deferAccessControllerCleanup) {
+ Runnable accessControllerCleanup;
synchronized (this) {
this.objectCreated = false;
this.initialized = false;
- onClose();
+ accessControllerCleanup = detachAccessController();
+ closeResourcesQuietly("resetting catalog");
Review Comment:
**[P2] Defer all blocking catalog cleanup outside the global lock**
`alterCatalogProps()` still holds `CatalogMgr`'s write lock when this
synchronized block calls `closeResourcesQuietly()`. That path can wait up to
two 60-second executor shutdown intervals and, for plugin catalogs,
synchronously calls external `Connector.close()` plus
connector-context/filesystem close; only the access-controller runnable is
deferred by this patch. One slow JDBC/filesystem/plugin teardown can therefore
still exclude every unrelated catalog CREATE/DROP/ALTER. The existing
controller-lock thread is fixed, but this is a distinct remaining cleanup
stage. Detach all old common/connector/context resources under the locks and
run their blocking closes afterward, then use a real latching
connector/common-resource test rather than mocking the entire catalog helper.
##########
fe/fe-core/src/main/java/org/apache/doris/mysql/privilege/AccessControllerManager.java:
##########
@@ -116,44 +134,112 @@ private void loadAccessControllerPlugins() {
}
public CatalogAccessController getAccessControllerOrDefault(String ctl) {
- CatalogAccessController catalogAccessController =
ctlToCtlAccessController.get(ctl);
- if (catalogAccessController != null) {
- return catalogAccessController;
+ if (InternalCatalog.INTERNAL_CATALOG_NAME.equals(ctl)) {
+ return defaultAccessController;
}
CatalogIf catalog =
Env.getCurrentEnv().getCatalogMgr().getCatalog(ctl);
if (catalog != null && catalog instanceof ExternalCatalog) {
+ CatalogAccessControllerEntry entry =
ctlToCtlAccessController.get(ctl);
+ if (entry != null && entry.catalogId == catalog.getId()) {
+ return entry.accessController;
+ }
lazyLoadCtlAccessController((ExternalCatalog) catalog);
- return ctlToCtlAccessController.get(ctl);
+ entry = ctlToCtlAccessController.get(ctl);
+ if (entry != null && entry.catalogId == catalog.getId()) {
+ return entry.accessController;
+ }
}
return defaultAccessController;
}
- private synchronized void lazyLoadCtlAccessController(ExternalCatalog
catalog) {
- if (ctlToCtlAccessController.containsKey(catalog.getName())) {
- return;
+ private void lazyLoadCtlAccessController(ExternalCatalog catalog) {
+ CatalogAccessControllerEntry staleEntry = null;
+ synchronized (this) {
+ if (!isCurrentCatalog(catalog)) {
+ return;
+ }
+ CatalogAccessControllerEntry entry =
ctlToCtlAccessController.get(catalog.getName());
+ if (entry != null && entry.catalogId == catalog.getId()) {
+ return;
+ }
+ if (entry != null &&
ctlToCtlAccessController.remove(catalog.getName(), entry)) {
+ staleEntry = entry;
+ }
}
+ closeEntry(catalog.getName(), staleEntry);
+
catalog.initAccessController(false);
- if (!ctlToCtlAccessController.containsKey(catalog.getName())) {
- ctlToCtlAccessController.put(catalog.getName(),
defaultAccessController);
+
+ CatalogAccessControllerEntry displaced = null;
+ boolean stillCurrent;
+ synchronized (this) {
+ stillCurrent = isCurrentCatalog(catalog);
+ if (stillCurrent) {
+ CatalogAccessControllerEntry entry =
ctlToCtlAccessController.get(catalog.getName());
+ if (entry == null || entry.catalogId != catalog.getId()) {
Review Comment:
**[P1] Fence lazy publication across same-ID property resets**
`stillCurrent` proves only that the catalog object/id is still registered. A
lazy load can snapshot the old properties, pause, and then an `ALTER CATALOG`
on that same object can update properties and reset/detach while no entry
exists. When the old load resumes, this block can publish the default alias
(for blank-to-Ranger ALTER), or the later custom path can publish a controller
built from old properties; the matching id then makes every future lookup
accept it without retrying the new configuration. This differs from the
existing DROP/recreate thread because ALTER preserves object and id. Carry a
reset/property generation through the snapshot and publication checks, close
stale candidates, and add latch tests for blank-to-custom and old-to-new
controller properties.
--
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]
