github-actions[bot] commented on code in PR #66269:
URL: https://github.com/apache/doris/pull/66269#discussion_r3710085853
##########
fe/fe-core/src/main/java/org/apache/doris/mysql/privilege/AccessControllerManager.java:
##########
@@ -172,8 +174,15 @@ public void removeAccessController(String ctl) {
if (StringUtils.isBlank(ctl)) {
return;
}
- if (ctlToCtlAccessController.containsKey(ctl)) {
- ctlToCtlAccessController.remove(ctl);
+ CatalogAccessController accessController =
ctlToCtlAccessController.remove(ctl);
Review Comment:
**[P1] Make controller teardown generation- and ownership-aware**
`ctlToCtlAccessController` is keyed only by catalog name, but DROP/rename
detaches the old catalog before this cleanup runs. A same-name replacement can
therefore reuse the old controller or install its new controller before this
`remove(ctl)`, after which old cleanup removes/closes whichever generation is
current. Conversely, a query that captured the old catalog can finish lazy
construction after DROP saw an empty map and publish a Ranger controller with
no remaining owner. The no-custom path also caches the shared
`defaultAccessController`, so this block closes the global controller still
used by `internal`. Track catalog generation and exclusive ownership, remove by
expected identity, never close fallback aliases, and add DROP/recreate,
post-DROP publication, and shared-default latch tests.
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/authorizer/ranger/hive/RangerHiveAuditHandler.java:
##########
@@ -239,17 +239,22 @@ public void processResults(Collection<RangerAccessResult>
results) {
}
}
- public void flushAudit() {
- for (AuthzAuditEvent auditEvent : auditEvents) {
- if (deniedExists && auditEvent.getAccessResult() != 0) { // if
deny exists, skip logging for allowed results
+ public synchronized void flushAudit() {
Review Comment:
**[P2] Emit audit events outside the producer monitor**
Both authorization callbacks and `addAuthzAuditEvent()` need this handler's
monitor, but the synchronized method keeps it while every
`super.logAuthzAudit()` call invokes the configured provider. A slow or
saturated audit sink can therefore stall all DB/table/column and mask/filter
authorization callbacks for the whole 20-second batch. Snapshot/reset the
buffer under a short synchronized section, then serialize provider emission
with a separate flush lock outside the producer monitor. Add a latch test
proving a producer can enqueue while provider delivery is blocked.
##########
fe/fe-core/src/main/java/org/apache/doris/mysql/privilege/AccessControllerManager.java:
##########
@@ -172,8 +174,15 @@ 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) {
+ try {
+ accessController.close();
Review Comment:
**[P2] Move registered controller shutdown outside the catalog lock**
`alterCatalogProps()` keeps CatalogMgr's global write lock through
`modifyCatalogProps()` and `resetToUninitialized()`, so it reaches this
synchronous `close()` while every unrelated catalog mutation is excluded.
Ranger close can wait for in-flight authorization, drain the configured audit
provider, and run plugin cleanup; a directory-loaded implementation can perform
equally blocking external work. Detach and fence the old controller as part of
the locked state transition, then wait for users and shut it down after
releasing the catalog lock. A blocking-close latch test should show another
catalog CREATE/DROP can proceed while ALTER teardown is stalled.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/plugin/PluginDrivenExternalCatalog.java:
##########
@@ -1519,8 +1519,8 @@ private void
closeConnectorContextQuietly(DefaultConnectorContext context) {
}
@Override
- public void onClose() {
- super.onClose();
+ protected void closeResources() {
Review Comment:
**[P1] Contain unchecked connector failures at this teardown stage**
This registered teardown still catches only `IOException` from the external
`Connector` SPI. If `connector.close()` throws a runtime failure, ALTER has
already mutated live properties but has not written its edit log, while DROP
has already removed/journaled the catalog and now skips connector-context,
constraint, cache, and query-stat cleanup. Failed CREATE preserves its primary
exception at the outer boundary but still skips the context stage. Make each
teardown stage independently non-throwing, detach/null owned fields in
`finally`, and ensure later catalog cleanup always runs. Add throwing-connector
tests for failed CREATE, registered ALTER, and DROP.
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/authorizer/ranger/hive/RangerHiveAuditHandler.java:
##########
@@ -239,17 +239,22 @@ public void processResults(Collection<RangerAccessResult>
results) {
}
}
- public void flushAudit() {
- for (AuthzAuditEvent auditEvent : auditEvents) {
- if (deniedExists && auditEvent.getAccessResult() != 0) { // if
deny exists, skip logging for allowed results
+ public synchronized void flushAudit() {
+ Collection<AuthzAuditEvent> eventsToFlush = new
ArrayList<>(auditEvents);
+ boolean deniedExistsForEvents = deniedExists;
+ auditEvents.clear();
Review Comment:
**[P1] Preserve the audit batch and periodic flusher on delivery failure**
This clears the complete snapshot before the first provider call. `close()`
already treats an unchecked failure from `flushAudit()` as possible, but the
periodic `RangerHiveAuditLogFlusher.run()` has no such boundary; one exception
therefore discards the drained events and causes `scheduleAtFixedRate` to
suppress every later execution for this catalog. Future decisions then
accumulate silently until close. Retain or requeue every event not confirmed
delivered, contain/log provider failures at the scheduled-task boundary, and
test a throw-once provider followed by a successful retry and later tick.
--
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]
