Skip to content

feat(transaction): Add TransactionAction and related classes #1420

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.

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
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

CTTY
Copy link
Contributor

@CTTY CTTY commented Jun 7, 2025

Which issue does this PR close?

This is a part of the effort to refactor transaction commit path and enable retry for write operations.
Please find the POC here: #1400

Related Issues:

What changes are included in this PR?

  • Add TransactionAction, ActionCommit, and ApplyTransactionAction
  • Add actions field in Transaction

Are these changes tested?

Added unit tests

Copy link
Contributor

@liurenjie1024 liurenjie1024 left a comment

Choose a reason for hiding this comment

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

Thanks @CTTY for this pr, generally looks good! Just some minor suggestions.

@CTTY CTTY requested a review from liurenjie1024 June 9, 2025 18:41
fn apply(self, tx: Transaction) -> Result<Transaction>;
}

impl<T: TransactionAction + 'static> ApplyTransactionAction for T {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this apply will have different implementation? If not, maybe we can provide a "apply" function in Transaction directly, like:

let action1 ...
let action2 ...
 let tx = Transaction::new(&table);
tx.apply(action1).apply(action2)..

Looks like these two method implement the same effect and just a function in Transaction maybe more clear.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's an auto implementation and will be apply to all transaction actions.

Copy link
Contributor

Choose a reason for hiding this comment

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

This enables fluent api call like this:

let mut tx = action.add_file("x1")
.add_file("x2")
.apply(tx);

Copy link
Contributor

@liurenjie1024 liurenjie1024 left a comment

Choose a reason for hiding this comment

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

Thanks @CTTY , LGTM!

@liurenjie1024
Copy link
Contributor

Let's wait for a while to see if others have concerns.

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.

3 participants