CAMEL-24242: camel-aws2-athena - stop ignoring thread interruption while polling a query#25032
CAMEL-24242: camel-aws2-athena - stop ignoring thread interruption while polling a query#25032oscerd wants to merge 1 commit into
Conversation
…ile polling a query Athena2QueryHelper.interrupted gates both shouldAttempt() and shouldWait(), but nothing assigned it any more, so both guards were dead code. CAMEL-20297 had added interrupt handling in doWait(); the CAMEL-22949 migration to Camel's Task API (commit 1b0fca1) replaced the Thread.sleep() block along with the catch clause that was its only writer, silently reverting it. ForegroundTask.run() does restore the interrupt status and return false, but doWait() discarded that result. The consequence was worse than a missed shutdown signal: once the interrupt status is set, every subsequent Thread.sleep() inside the task returns immediately, so the polling loop spun with no delay, issuing GetQueryExecution calls as fast as the API allowed until waitTimeout elapsed. Capture the return value and record the interruption, restoring CAMEL-20297's behaviour on top of the Task API. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Andrea Cosentino <ancosen@gmail.com> (cherry picked from commit f7c0fe4)
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Clean and correct backport of the interrupt-handling fix from PR #25029 (main) to camel-4.18.x. The regression analysis is accurate — commit 1b0fca17c04 (CAMEL-22949, Task API migration) replaced Thread.sleep() with Tasks.foregroundTask()...run() but dropped the catch (InterruptedException) block, leaving the interrupted field as a dead variable.
The fix correctly captures the ForegroundTask.run() return value and sets this.interrupted = true when the task did not complete and the thread is interrupted, restoring the existing shouldWait() and shouldAttempt() guards. The test is well-structured with proper cleanup.
Minor note (non-blocking): The new test uses AssertJ (assertThat) while the existing 22 tests in the file use JUnit assertions (assertTrue/assertFalse/assertEquals). The PR description acknowledges this and keeps existing assertions untouched, which is reasonable.
LGTM.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of Guillaume Nodet
3 participants
Backport of #25029 to
camel-4.18.x.Athena2QueryHelper.interruptedgates bothshouldAttempt()andshouldWait()but has no writer, so both guards are dead code. CAMEL-20297 had added the
interrupt handling in
doWait(); the CAMEL-22949 Task-API migration(
1b0fca17c0) dropped thecatch (InterruptedException)block that was its onlywriter.
Because
ForegroundTask.run()restores the interrupt status anddoWait()discarded its return value, an interrupted poll loop spins with no delay,
hammering
GetQueryExecutionuntilwaitTimeoutelapses. The fix records theinterruption so both loops bail out.
This branch carries the CAMEL-22949 migration, so it is affected.
camel-4.14.xis not — it still has the original
Thread.sleep()with itscatchblock,so no backport is needed there.
Cherry-picked cleanly;
Athena2QueryHelperTestpasses 23/23 on this branch.No upgrade-guide entry — this restores intended behaviour with no user-visible
configuration change.
Claude Code on behalf of oscerd