czy006 commented on code in PR #4331:
URL: https://github.com/apache/amoro/pull/4331#discussion_r3826730413
##########
amoro-ams/src/main/java/org/apache/amoro/server/optimizing/OptimizingQueue.java:
##########
@@ -775,7 +775,10 @@ private void resetTask(TaskRuntime<RewriteStageTask>
taskRuntime) {
@Override
public boolean isClosed() {
Review Comment:
Thanks for the question. `isClosed()` is not a shortcut for `getStatus() ==
ProcessStatus.CLOSED` — it is the predicate "this process was terminated
out-of-band and must reject late task results", and it intentionally spans two
statuses:
- `CLOSED` — written by `close(false)` (e.g. the table is released from the
queue / the optimizer group changes) and by the partial-commit completion path
in `commit()`;
- `KILLED` — the terminal state this class already treats as "process is
gone" in `poll()` (`status != KILLED && status != FAILED`), and the recovery
constructor restores the status from the persisted `TableProcessMeta`, so it
can carry whatever the process framework persisted.
That is why simply removing it and comparing `getStatus()` at the call site
does not express the intent:
- comparing `getStatus() == ProcessStatus.CLOSED` would silently lose the
`KILLED` arm;
- comparing `getStatus() == CLOSED || getStatus() == KILLED` just re-creates
this predicate inline, without a name.
For context, this is also exactly how the bug was introduced: #3257 migrated
the old `OptimizingProcess.Status` check correctly (`status ==
ProcessStatus.CLOSED`), then #3486 changed the *readers* (`poll()`, the
recovery check, `isClosed()`) from `CLOSED` to `KILLED` while leaving `close()`
writing `CLOSED`. Since nothing in this class assigns `KILLED`, `isClosed()`
has been permanently false and the guard in `acceptResult()` dead ever since.
Keeping one named predicate for "results are no longer accepted" — instead of
scattered raw status comparisons — is what prevents this kind of writer/reader
drift from recurring.
On the naming — fair point that `isClosed()` covering `KILLED` can read as a
`CLOSED` shortcut at first glance. I'd like to keep this hotfix minimal (just
restoring the dead guard) and treat any rename (e.g. `isTerminated()`) as a
follow-up if you think it is worth it, since it would touch the public
`OptimizingProcess` interface.
--
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]
