Skip to content

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

.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
@ZENOTME ZENOTME deleted the apply_metadata branch May 10, 2025 10:01
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