FrankChen021 commented on code in PR #20424:
URL: https://github.com/apache/druid/pull/20424#discussion_r4104442242
##########
extensions-contrib/rabbit-stream-indexing-service/src/main/java/org/apache/druid/indexing/rabbitstream/RabbitStreamRecordSupplier.java:
##########
@@ -347,6 +348,7 @@ public void handle(MessageHandler.Context context, Message
message)
}
}
catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
Review Comment:
[P1] Restoring the flag prevents the requested consumer shutdown
**Finding:** This catch restores the interrupt flag and then calls
stopBackgroundFetch(). That method immediately invokes
stateSemaphore.acquire(), which throws without acquiring when the flag is set;
its own catch swallows the second interruption and returns. An interruption
while queue.offer() is blocked therefore leaves isRunning true and the Rabbit
consumers open, so the supplier can continue fetching after the shutdown signal.
**Suggestion:** Perform the interruptible cleanup before restoring the
status, or make stopBackgroundFetch complete uninterruptibly and restore the
status after it has reset isRunning and closed the consumers.
##########
extensions-core/druid-basic-security/src/main/java/org/apache/druid/security/basic/CommonCacheNotifier.java:
##########
@@ -141,6 +141,7 @@ public void start()
LOG.debug(callerName + ":Received responses for cache update
notifications.");
}
catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
Review Comment:
[P1] Notification-response interrupts can still strand the notifier
**Finding:** The new outer catch only handles interruption from
updateQueue.take(). The Futures.allAsList(futures).get(...) call in the inner
try also throws InterruptedException, but the surrounding catch (Exception)
handles it, clears the flag, and lets the while loop block on
updateQueue.take() again. shutdownNow() during response waiting can therefore
still leave this notifier thread blocked indefinitely.
**Suggestion:** Handle InterruptedException from the response wait
separately so the loop exits while preserving the status, before the generic
exception logging path.
--
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]
