github-actions[bot] commented on code in PR #66269:
URL: https://github.com/apache/doris/pull/66269#discussion_r3702463493
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/CatalogMgr.java:
##########
@@ -252,7 +252,7 @@ private void createCatalogImpl(CatalogIf catalog, String
catalogName,
try {
if (nameToCatalog.containsKey(catalog.getName())) {
// Close the already-constructed catalog to release connector
resources.
- catalog.onClose();
+ catalog.onCreateFailure();
Review Comment:
**[P2] Close the duplicate candidate after releasing the catalog lock**
The loser in the documented same-name race is closed while the global
catalog write lock is still held. A JDBC candidate has already run connectivity
validation and started its Hikari pool; the pinned HikariCP 6.0.0 shutdown can
await the connection-adder and multiple 10-second executor drains, so one
duplicate `CREATE CATALOG` can block every catalog create/drop/alter behind
external pool cleanup. Decide/detach the loser under the lock, preserve the
duplicate or `IF NOT EXISTS` result, then call `onCreateFailure()` after
unlocking; a blocking-close latch test should prove unrelated catalog mutation
can proceed.
##########
fe/fe-core/src/main/java/org/apache/doris/mysql/privilege/AccessControllerManager.java:
##########
@@ -172,8 +174,9 @@ public void removeAccessController(String ctl) {
if (StringUtils.isBlank(ctl)) {
return;
}
- if (ctlToCtlAccessController.containsKey(ctl)) {
- ctlToCtlAccessController.remove(ctl);
+ CatalogAccessController accessController =
ctlToCtlAccessController.remove(ctl);
+ if (accessController != null) {
+ accessController.close();
Review Comment:
**[P1] Contain controller-close failures before catalog state escapes**
This invokes a directory-loaded controller's new `close()` contract without
any failure boundary. If it throws a runtime exception,
`ExternalCatalog.onClose()` never reaches `closeResources()`. More seriously,
`ALTER CATALOG SET PROPERTIES` has already mutated the live `CatalogProperty`
before reset reaches this call, but its edit log is written only after the call
returns, so the leader can retain unjournaled properties; DROP has already
removed/journaled the catalog and then skips the remaining
constraint/cache/stat cleanup. Please make manager-side close
best-effort/non-throwing, ensure later cleanup runs in `finally`, and add
throwing-controller ALTER/DROP tests.
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/authorizer/ranger/hive/RangerHiveAccessController.java:
##########
@@ -64,7 +66,20 @@ public RangerHiveAccessController(Map<String, String>
properties,
hivePlugin = new RangerHivePlugin(serviceName,
rangerAuthContextListener);
auditHandler = new RangerHiveAuditHandler(hivePlugin.getConfig());
// start a timed log flusher
- logFlushTimer.scheduleAtFixedRate(new
RangerHiveAuditLogFlusher(auditHandler), 10, 20L, TimeUnit.SECONDS);
+ logFlushFuture = LOG_FLUSH_TIMER.scheduleAtFixedRate(
+ new RangerHiveAuditLogFlusher(auditHandler), 10, 20L,
TimeUnit.SECONDS);
+ }
+
+ @Override
+ public void close() {
+ if (logFlushFuture != null) {
+ logFlushFuture.cancel(true);
+ logFlushFuture = null;
+ }
+ if (hivePlugin != null) {
+ hivePlugin.cleanup();
Review Comment:
**[P1] Fence in-flight checks before cleaning up Ranger**
`getAccessControllerOrDefault()` returns the controller and every
DB/table/column, mask, and row-filter path invokes it without a lifecycle
guard. A concurrent `DROP CATALOG` or property reset can remove this object
here, call `cleanup()`, and null `hivePlugin` after a query has acquired it but
before `checkPrivilege(s)` dereferences that field at lines 120/129. The query
then gets an NPE or uses Ranger after its policy engine was released. Please
add a read-side lease/guard spanning the complete authorization call, defer
cleanup until acquired users drain, and cover the interleaving with latches.
This is distinct from the existing failed-candidate/winner-removal thread: it
is registered teardown racing an already-started check.
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/authorizer/ranger/hive/RangerHiveAccessController.java:
##########
@@ -64,7 +66,20 @@ public RangerHiveAccessController(Map<String, String>
properties,
hivePlugin = new RangerHivePlugin(serviceName,
rangerAuthContextListener);
auditHandler = new RangerHiveAuditHandler(hivePlugin.getConfig());
// start a timed log flusher
- logFlushTimer.scheduleAtFixedRate(new
RangerHiveAuditLogFlusher(auditHandler), 10, 20L, TimeUnit.SECONDS);
+ logFlushFuture = LOG_FLUSH_TIMER.scheduleAtFixedRate(
+ new RangerHiveAuditLogFlusher(auditHandler), 10, 20L,
TimeUnit.SECONDS);
+ }
+
+ @Override
+ public void close() {
+ if (logFlushFuture != null) {
+ logFlushFuture.cancel(true);
Review Comment:
**[P1] Drain pending audit records before canceling the flusher**
`RangerHiveAuditHandler` buffers audited decisions, and this scheduled task
is the only caller of `flushAudit()`. If a catalog authorizes a request and is
dropped or reset before the first 10-second tick (or between 20-second ticks),
canceling the future here leaves those records with no consumer;
`hivePlugin.cleanup()` does not drain this Doris-owned handler. Please fence
new/in-flight checks, serialize cancellation with any running flush, perform
one final drain before plugin cleanup, and test that a pre-tick event is
emitted exactly once.
--
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]
