github-actions[bot] commented on code in PR #66269:
URL: https://github.com/apache/doris/pull/66269#discussion_r3714720001
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/authorizer/ranger/hive/RangerHiveAccessController.java:
##########
@@ -64,7 +74,41 @@ 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() {
+ lifecycleLock.writeLock().lock();
+ try {
+ if (closed) {
+ return;
+ }
+ closed = true;
+ if (logFlushFuture != null) {
+ logFlushFuture.cancel(false);
+ logFlushFuture = null;
+ }
+ // flushAudit atomically drains the handler. This preserves events
produced before close without
+ // racing the periodic flusher or re-emitting events it has
already sent.
+ try {
+ auditHandler.flushAudit();
Review Comment:
**[P1] Keep a consumer alive when the final audit flush fails**
This cancels and clears the only scheduled consumer before attempting the
final `flushAudit()`. The handler now retains a failed head in
`pendingAuditEvents`, but if the configured provider throws once here, this
catch only logs before plugin cleanup; the manager then drops the controller
and every later `close()` returns because `closed` is already set. An event
authorized just before ALTER/DROP is therefore permanently lost even if the
provider would succeed on retry. Please transfer a failed final batch to a
consumer that outlives the controller (or otherwise retry before releasing
ownership), and add a close test with a throw-once provider.
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/authorizer/ranger/hive/RangerHiveAccessController.java:
##########
@@ -42,17 +44,25 @@
import java.util.Date;
import java.util.List;
import java.util.Map;
+import java.util.Optional;
import java.util.Set;
+import java.util.concurrent.ScheduledFuture;
import java.util.concurrent.ScheduledThreadPoolExecutor;
import java.util.concurrent.TimeUnit;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
import java.util.stream.Collectors;
public class RangerHiveAccessController extends RangerAccessController {
private static final Logger LOG =
LogManager.getLogger(RangerHiveAccessController.class);
- private static ScheduledThreadPoolExecutor logFlushTimer =
ThreadPoolManager.newDaemonScheduledThreadPool(1,
+ private static final ScheduledThreadPoolExecutor LOG_FLUSH_TIMER =
ThreadPoolManager.newDaemonScheduledThreadPool(1,
Review Comment:
**[P2] Isolate audit progress across Ranger catalogs**
The new one-shot flusher removes the previous forever loop and the
producer-lock fix lets other callbacks enqueue, but every controller still
performs synchronous provider delivery on this static one-thread scheduler. If
catalog A's provider stalls, the sole worker cannot start catalog B's
already-due task, so B's audit latency and buffer growth are unbounded.
`ThreadPoolManager.newDaemonScheduledThreadPool()` explicitly reserves this
helper for short fixed-rate tasks. Please keep the timer callback short and
provide independent, bounded delivery progress with per-handler serialization,
plus a two-controller latch test where B flushes while A's provider is blocked.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/plugin/PluginDrivenExternalCatalog.java:
##########
@@ -1513,21 +1513,29 @@ private void
closeConnectorContextQuietly(DefaultConnectorContext context) {
}
try {
context.close();
- } catch (IOException e) {
+ } catch (Throwable e) {
LOG.warn("Failed to close connector context filesystem for catalog
{}", name, e);
}
}
@Override
- public void onClose() {
- super.onClose();
- if (connector != null) {
+ protected void closeResources() {
+ try {
+ super.closeResources();
+ } catch (Throwable e) {
+ LOG.warn("Failed to close common resources for plugin-driven
catalog {}", name, e);
+ }
+
+ // Detach every stage before invoking external code. A throwing
connector must not remain reachable
+ // for another close attempt or prevent the connector-context stage
from running.
+ Connector connectorToClose = connector;
+ connector = null;
+ if (connectorToClose != null) {
try {
- connector.close();
- } catch (IOException e) {
+ connectorToClose.close();
Review Comment:
**[P2] Pin the connector loader before detaching cleanup**
This still invokes a directory-loaded connector under the FE caller's TCCL.
A child-first plugin whose `close()` uses `ServiceLoader`, reflective by-name
loading, or starts cleanup workers can therefore fail with an unchecked
classloading error. The new order makes that materially worse on ALTER:
`connector` is nulled first and every `Throwable` is swallowed, so the DDL can
succeed after permanently abandoning the plugin's resources. Please invoke
connector lifecycle callbacks under the connector's defining classloader with
restoration in `finally`, reuse the helper for the first-initialization
replacement close, and add a directory-loaded fake whose `close()` verifies
TCCL and restoration.
--
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]
