Skip to content

Replace AbstractFateStore.verifyReserved() with new method FateMutator.requireReserved() - #6521

Open
Amemeda wants to merge 5 commits into
apache:mainfrom
Amemeda:4908-require-reserved
Open

Replace AbstractFateStore.verifyReserved() with new method FateMutator.requireReserved()#6521
Amemeda wants to merge 5 commits into
apache:mainfrom
Amemeda:4908-require-reserved

Conversation

@Amemeda

@Amemeda Amemeda commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Add new method to FateMutator.java that requires a FateId to be reserved for certain transactions. This method will replace AbstractFateStore.verifyReservedAndNotDeleted() implementation in UserFateStore.java.

  • Added FateMutator.requireReserved() and its implementation in FateMutatorImpl.java. requireReserved() requires a transaction to be reserved with a specific FateStore.FateReservation.
  • Removed usages of verifyReservedandNotDeleted() from UserFateStore.java methods. To replace its implementation, new method requireReserved() was appended onto existing newMutators in push(), pop(), setStatus(), setTransactionInfo(), delete(), and forceDelete(). For methods top(), getStack(), getTransactionInfo(), and timeCreated(), verifyReservedandNotDeleted() simply was removed.

Closes #4908

@Amemeda
Amemeda marked this pull request as draft August 31, 2026 16:09
@Amemeda

Amemeda commented Sep 1, 2026

Copy link
Copy Markdown
Contributor Author

(OUTDATED)
Since verifyReservedAndNotDeleted() was removed from UserFateStore.java, new requireReserved() was implemented in its place on the FateMutator side. However, in 4 UserFateStore.java methods (top, getStack, getTransactionInfo, timeCreated) there is no corresponding fateMutator method being called where requireReserved() should be added. Unsure how to replace implementation for these 4 methods.

@Amemeda Amemeda changed the title Replace AFS.verifyReserved with a condition Replace AbstractFateStote.verifyReserved() with new method FateMutator.requireReserved() Sep 1, 2026
@Amemeda Amemeda changed the title Replace AbstractFateStote.verifyReserved() with new method FateMutator.requireReserved() Replace AbstractFateStore.verifyReserved() with new method FateMutator.requireReserved() Sep 1, 2026
@Amemeda
Amemeda marked this pull request as ready for review September 3, 2026 17:50
@Override
public Repo<T> top() {
verifyReservedAndNotDeleted(false);
newMutator(fateId).requireReserved(reservation);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Pretty sure all these spots can just be removed instead of replaced. verifyReservedAndNotDeleted(false) is a no-op. Also this newMutator(fateId).requireReserved(reservation); creates a conditional mutation but never sends it which we don't want or need to do.

verifyReservedAndNotDeleted(true);

newMutator(fateId).putStatus(status).mutate();
newMutator(fateId).requireReserved(reservation).putStatus(status).mutate();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
newMutator(fateId).requireReserved(reservation).putStatus(status).mutate();
newReservedMutator().putStatus(status).mutate();

It might be nice to create a helper in this class that returns newMutator(fateId).requireReserved(reservation). Then we can just replace the newMutator() calls with newReservedMutator().

It also has the benefit of making the code shorter and we can add a check in this new helper method like
Preconditions.checkState(!deleted, Attempted write on deleted FATE transaction: " + fateId); for completeness

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.

Replace AFS.verifyReserved with a condition

2 participants