czy006 commented on code in PR #4261:
URL: https://github.com/apache/amoro/pull/4261#discussion_r3549937275
##########
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:
I think this still needs an attempt/generation guard for completions.
`TaskRuntime.complete()` currently treats a completion as current as long as
`(token, threadId)` match and the task is in `ACKED`. That covers the new test
where the stale completion arrives while the rescheduled task is still
`SCHEDULED`, but it does not cover this sequence:
1. Task T is scheduled to optimizer token A, thread 1.
2. Optimizer acks T and starts the first attempt.
3. AMS resets T back to `PLANNED`.
4. The same optimizer thread polls T again, so T is scheduled again to token
A, thread 1.
5. Optimizer acks the second attempt, so T is now `ACKED` again.
6. A delayed completion from the first attempt reaches AMS.
At step 6, the current checks cannot distinguish the first attempt from the
second attempt, because the result only carries `taskId/threadId` and the live
task state also has the same `token/threadId` with status `ACKED`. The stale
completion can therefore be accepted as the result of the new attempt.
The normal optimizer loop is mostly serial, so this may be rare, but the
server-side protocol is still not generation-safe under delayed RPCs, HA/client
timeout/retry, or shutdown drain edges.
A safer fix would be to add an attempt id / schedule generation when the
task is scheduled, include it in `OptimizingTask`, and require `completeTask`
to return the same generation before accepting the result. Alternatively, the
code needs a stronger guarantee that a reset task cannot be rescheduled to the
same optimizer thread while a previous completion may still arrive.
cc WDYT? @j1wonpark
--
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]
