Skip to content

[hotfix][Optimizer] Recognize CLOSED optimizing processes #4331

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
czy006 wants to merge 2 commits into
base: master
Choose a base branch
from
+6 −1
Open

[hotfix][Optimizer] Recognize CLOSED optimizing processes #4331

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
96a97b2
[hotfix][Optimizer] Make isClosed() recognize the CLOSED status
czy006 Aug 16, 2026
a4461d5
Merge branch 'master' into codex/hotfix-optimizer-closed-status
czy006 Aug 21, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion amoro-ams/src/main/java/org/apache/amoro/server/optimizing/OptimizingQueue.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -775,7 +775,10 @@ private void resetTask(TaskRuntime<RewriteStageTask> taskRuntime) {

@Override
public boolean isClosed() {

@xxubai xxubai Aug 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering why we need isClosed() if we already have getStatus(). If it represents something other than the process status, we should either rename it to make its semantics clearer or remove it and use getStatus() instead.

@czy006 czy006 Aug 21, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return status == ProcessStatus.KILLED;
// close() sets CLOSED (KILLED is reserved for kill flows); checking only KILLED made
// this predicate permanently false, so the acceptResult guard against late results on a
// closed process could never fire.
return status == ProcessStatus.CLOSED || status == ProcessStatus.KILLED;
}

@Override
Expand Down
2 changes: 2 additions & 0 deletions amoro-ams/src/test/java/org/apache/amoro/server/optimizing/TestOptimizingQueue.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -858,6 +858,8 @@ public void testProcessCloseKeepsLastOptimizedSnapshotId() {
// Close process without success (simulates group change / forced termination)
process.close(false);

Assert.assertTrue(process.isClosed());

// lastOptimizedSnapshotId and lastOptimizedChangeSnapshotId should NOT be updated
Assert.assertEquals(snapshotIdBeforePlanning, tableRuntime.getLastOptimizedSnapshotId());
Assert.assertEquals(
Expand Down
Loading