Skip to content

feat: add apply in transaction to support stack action #949

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

Merged
merged 6 commits into from
Apr 23, 2025

Conversation

ZENOTME
Copy link
Contributor

@ZENOTME ZENOTME commented Feb 7, 2025

@ZENOTME
Copy link
Contributor Author

ZENOTME commented Feb 7, 2025

cc @Fokko @liurenjie1024 @Xuanwo @sdd

@ZENOTME
Copy link
Contributor Author

ZENOTME commented Feb 13, 2025

Hi @liurenjie1024, I think we should resolve this PR first before working on #964. It will affect the interface in transactions.

@ZENOTME ZENOTME force-pushed the apply_metadata branch 4 times, most recently from 5e8e0b0 to 9b7ef77 Compare March 11, 2025 07:26
@ZENOTME
Copy link
Contributor Author

ZENOTME commented Mar 11, 2025

I think this PR is ready to review. cc @Fokko @liurenjie1024 @Xuanwo @sdd

@@ -261,7 +286,7 @@ impl<'a> FastAppendAction<'a> {
let mut manifest_stream = self
.snapshot_produce_action
.tx
.table
.current_table
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I realized that there are some functions based on the table. And storing only current metadata may cause inconsistencies easily in the future. So change to store current table instead of metadata

Copy link
Contributor

@jonathanc-n jonathanc-n left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @ZENOTME

@jonathanc-n
Copy link
Contributor

I think we should try to move this along. cc @liurenjie1024 @Fokko @sdd

@liurenjie1024
Copy link
Contributor

I think we should try to move this along. cc @liurenjie1024 @Fokko @sdd

Thanks, I'll take a look recently.

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 @ZENOTME for this pr, generally LGTM! Just one minor suggestion.

@liurenjie1024
Copy link
Contributor

cc @ZENOTME Would you help to resolve the conflicts?

@ZENOTME
Copy link
Contributor Author

ZENOTME commented Apr 22, 2025

cc @ZENOTME Would you help to resolve the conflicts?

Sure.

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 @ZENOTME for this pr, LGTM!

@liurenjie1024 liurenjie1024 merged commit 3e1d59d into apache:main Apr 23, 2025
17 checks passed
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.

Add apply interface in transaction
3 participants