FrankChen021 commented on code in PR #20072:
URL: https://github.com/apache/druid/pull/20072#discussion_r3836152739
##########
processing/src/test/java/org/apache/druid/segment/join/PostJoinCursorTest.java:
##########
@@ -237,7 +237,7 @@ private void testAdvance(boolean withInterruption) throws
IOException, Interrupt
joinCursorThread.setUncaughtExceptionHandler(exceptionHandler);
joinCursorThread.start();
- countDownLatch.await(1, TimeUnit.SECONDS);
+ assertTrue(countDownLatch.await(1, TimeUnit.SECONDS));
Review Comment:
[P1] Timeout assertion can leak the infinite test thread
If the latch times out, assertTrue throws before
joinCursorThread.interrupt(). The test cursor reports isDone() as false, so the
non-daemon thread can loop indefinitely and hang the test JVM.
##########
indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisor.java:
##########
@@ -2060,13 +2060,25 @@ public void runInternal()
logDebugReport();
}
catch (Exception e) {
- stateManager.recordThrowableEvent(e);
- if (e instanceof StreamException) {
- // When a StreamException is thrown, the error message is more useful
than the stack trace in telling what's wrong.
- log.makeAlert("Exception in supervisor run loop for supervisor[%s] for
dataSource[%s]: [%s]",
- supervisorId, dataSource, e.getMessage()).emit();
+ if (e instanceof ExecutionException || e instanceof
InterruptedException) {
Review Comment:
[P1] Task failures still reach unhealthy-state accounting
FutureUtils.coalesce converts task RPC failures to Either.error, and
existing checkpoint/pause callbacks record those errors before this catch;
wrapped failures can also bypass this type check. Repeated recoverable task
failures can therefore still increment failure state and trip
UNHEALTHY_SUPERVISOR.
##########
indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisor.java:
##########
@@ -2060,13 +2060,25 @@ public void runInternal()
logDebugReport();
}
catch (Exception e) {
- stateManager.recordThrowableEvent(e);
- if (e instanceof StreamException) {
- // When a StreamException is thrown, the error message is more useful
than the stack trace in telling what's wrong.
- log.makeAlert("Exception in supervisor run loop for supervisor[%s] for
dataSource[%s]: [%s]",
- supervisorId, dataSource, e.getMessage()).emit();
+ if (e instanceof ExecutionException || e instanceof
InterruptedException) {
Review Comment:
[P2] Recoverable exits count as successful first runs
Skipping recordThrowableEvent leaves currentRunSuccessful true, while
finally calls markRunFinished(). An aborted first run can therefore set
atLeastOneSuccessfulRun and transition to RUNNING, bypassing first-run recovery
behavior.
--
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]
