Skip to content

Prohibit rollback of a procedure which has passed its commit point - #18909

Open
alpass163 wants to merge 3 commits into
apache:masterfrom
alpass163:cyb/prohibit_rollback_in_procedure
Open

alpass163 wants to merge 3 commits into
apache:masterfrom
alpass163:cyb/prohibit_rollback_in_procedure

Conversation

@alpass163

@alpass163 alpass163 commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Fix the procedure frame that prohibiting rollback of a procedure which has passed its commit point

  • Procedure.isRollbackSupported() lets a procedure declare that its executed states can no
    longer be undone. The executor asks it once, before any state is rolled back, and finishes
    such a procedure without touching its states; the client still sees the failure.
  • The decision is persisted by clearing the stack indexes only, so the procedure store format
    is unchanged and rolling upgrades are not affected.
  • Each table procedure declares its commit point: the state which applies its change on the
    ConfigNode (ADD_COLUMN, RENAME_TABLE, SET_PROPERTIES, ...), or the point after which
    the data is gone (DELETE_DATA).

@Caideyipi

Copy link
Copy Markdown
Collaborator
  1. Better reuse the original "isRollbackSupported", or else other people may not know which to override, and the incoming reviewers may be confused and let the wrong implementation passed too.
  2. If you have time, you can also seek are there any other procedures which does not support rollback previously and can be supported in some stages after this PR

@alpass163

Copy link
Copy Markdown
Contributor Author
  1. Better reuse the original "isRollbackSupported", or else other people may not know which to override, and the incoming reviewers may be confused and let the wrong implementation passed too.
  2. If you have time, you can also seek are there any other procedures which does not support rollback previously and can be supported in some stages after this PR

Ok, I reuse the original api

@Caideyipi Caideyipi left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Found a recovery lock leak and a small localization issue; details are inline.

Validation: the PR's five new state-machine tests pass. Two additional lock-recovery cases, including one using the real AddConfigNodeProcedure and LockQueue, fail on 754625cce26 and pass on the base d1aaa0dd909. This used targeted compilation and JUnit with cached dependencies; cluster integration tests were not run.

@Caideyipi Caideyipi left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The recovery lock leak and retry-operation localization issue from the previous review are fixed. I found no further blocking issues in the current change.

Validation: all five new state-machine tests and two targeted lock-release cases using the real AddConfigNodeProcedure and LockQueue pass on eed5db9. These used targeted compilation with cached dependencies; the local reactor run stopped before ConfigNode because DataNode Checkstyle ran out of heap. CI unit tests on Linux/Windows and zh-locale compilation passed.

One CI integration shard remains red: IoTDBPipePermissionIT.testSourcePermission observes unexpected replicated rows. This still needs investigation or a successful rerun before merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants