j1wonpark commented on code in PR #4261:
URL: https://github.com/apache/amoro/pull/4261#discussion_r3564260032
##########
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:
Good point, and agreed — implemented in
f46b07cec4d320bbc063d8b02ff037163bf1eaf4, following your option (a).
`TaskRuntime.schedule()` now stamps a monotonically increasing attempt id
(`task-attempt-id`) into the task properties, which reach the optimizer through
the existing `OptimizingTask.properties` map. The optimizer echoes it back in
`OptimizingTaskResult.summary`, and `complete()` ignores a completion whose
echoed attempt id does not match the current one — exactly the step-6 case you
described. The echo sits at the single point every completion passes through
(the poll loop in `OptimizerExecutor`), so the standalone/Flink/Spark
optimizers are all covered, including error results built by executor
subclasses.
No thrift schema change is needed — both maps are existing optional fields.
Compatibility:
- An older optimizer doesn't echo the id, so its results fall back to the
`(token, threadId, status)` checks: the same acceptance window as before this
PR, no regression.
- The attempt id is persisted with the task (the existing `properties`
column), so it stays monotonic across AMS restarts.
One remaining gap, for the record: the `ackTask` RPC cannot carry the
attempt id without a thrift signature change, so acks are still matched by
`(token, threadId, status)` only. Since an ack is sent once and not retried
(unlike completions, which are drained with retries on shutdown), the
completion path is where this ambiguity actually bites; I'd leave the ack side
out of this PR.
Tests: `TestOptimizingQueue#testStaleCompleteFromPreviousAttemptIsIgnored`
reproduces your exact sequence and asserts the stale completion is ignored
while the current attempt's own completion is accepted;
`TestOptimizerExecutor#testEchoTaskAttemptIdOnSuccess/OnFailure` cover the echo.
--
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]
