j1wonpark commented on code in PR #4261:
URL: https://github.com/apache/amoro/pull/4261#discussion_r3524722828
##########
amoro-ams/src/main/java/org/apache/amoro/server/optimizing/TaskRuntime.java:
##########
@@ -82,9 +86,22 @@ public SimpleFuture getCompletedFuture() {
}
void complete(OptimizerThread thread, OptimizingTaskResult result) {
+ // A completion for an already reset/rescheduled task is stale: the
execution it reports belongs
+ // to a round that the OptimizerKeeper has already torn down. Absorb it
gracefully instead of
+ // failing the (now illegal) state transition; the task will be
re-executed in its current
+ // round.
+ if (isStaleResponse(thread) || status != Status.ACKED) {
Review Comment:
Thanks for the careful review — that's a sharp observation, and you're
right. When the same task is re-scheduled to the same optimizer thread,
`schedule()` re-assigns the identical token/threadId, so `(token, threadId,
status)` has no way to tell a delayed completion of the previous round apart
from the current one.
For context on how I ended up leaving this window as-is in this PR: the
behavior is the same before and after this change. The removed `validThread`
relied on the same `(token, threadId)` key, and the state machine already
accepted a thread-matching completion only at `ACKED` — so the pre-existing
code accepted the same delayed completion in exactly the same window. This PR
changes how the surrounding cases fail (graceful reject/ignore instead of a
transaction error), but does not widen what gets accepted.
Closing the window completely would need a per-attempt discriminator that
the optimizer echoes back (e.g. an attempt id carried via the existing
`OptimizingTask.properties` / `OptimizingTaskResult.summary` maps, compared in
`complete()`), which requires changes on the optimizer client side as well.
Since this PR doesn't change what gets accepted, I'd prefer to keep that
improvement out of its scope and keep this PR focused on handling the stale
responses gracefully on the AMS side.
##########
amoro-ams/src/test/java/org/apache/amoro/server/TestDefaultOptimizingService.java:
##########
@@ -314,6 +317,31 @@ public void testAckAndCompleteTask() {
assertTaskCompleted(taskRuntime);
}
+ // Reproduces the EXACT path of issue #4235 end-to-end with the real
OptimizerKeeper: a live
+ // optimizer (the Toucher keeps heartbeating) polls a task but its ack is
delayed past
+ // OPTIMIZER_TASK_ACK_TIMEOUT (30s). The keeper, via the SCHEDULED +
ackTimeout branch of
+ // buildSuspendingPredication, resets the still-owned task to PLANNED. The
late ack then arrives
+ // and is rejected -- this is the "Task has been reset or not yet scheduled"
from the issue log,
+ // produced without any artificial retryTask() call.
+ @Test
+ public void testAckTimeoutResetThenLateAckRejected() throws
InterruptedException {
+ OptimizingTask task = optimizingService().pollTask(token, THREAD_ID);
+ Assertions.assertNotNull(task);
+ assertTaskStatus(TaskRuntime.Status.SCHEDULED); // polled but NOT acked
+
+ // ack-timeout is 30s; the optimizer stays alive (Toucher touches every
300ms), so this hits the
+ // SCHEDULED + ackTimeout branch rather than the optimizer-expired branch.
+ Thread.sleep(35000);
Review Comment:
Good point, fixed in 515be61e65045a110fa2fe1319dc6f9fe6e97833.
`OPTIMIZER_TASK_ACK_TIMEOUT` is now overridden in `AMSServiceTestBase` and the
fixed 35s sleep is replaced with a 100ms status-polling wait, so the test
finishes right after the keeper reset lands (~6s instead of 35s).
One finding from trying the suggested 1-2s value: the ack timeout has to
stay above `optimizer.polling-timeout` (3s). A `pollTask` blocking for its full
polling timeout would otherwise pick up the very task the keeper reset by
ack-timeout in the meantime — at 2s this broke `testPollTaskThreeTimes`, whose
final poll expects `null`. So I settled on 5s and left a comment in
`AMSServiceTestBase` documenting the constraint.
--
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]
