feat(transaction): Remove current_table, updates, and requirements from Transaction #1451
Conversation
|
|
||
| self.current_table | ||
| .with_metadata(Arc::new(metadata_builder.build()?.metadata)); | ||
| table.with_metadata(Arc::new(metadata_builder.build()?.metadata)); |
There was a problem hiding this comment.
Not related to this pr, and it's not mandatory. But I think it's better to change the with_metadata(mut self) to consume table rather than modify it.
There was a problem hiding this comment.
this is a very good point. I did notice people are confused with the usage of tx.apply in other discussions, and making with_metadata consume tables would require tx.apply to return Result<Table> instead of Result<()> and make it clearer on its usage.
I've included the change in this PR, please let me know what you think!
|
|
||
| async fn do_commit(&mut self, catalog: &dyn Catalog) -> Result<Table> { | ||
| let base_table_identifier = self.base_table.identifier().to_owned(); | ||
| let base_table_identifier = self.table.identifier().to_owned(); |
There was a problem hiding this comment.
Why we need to call to_owned? The load_table accepts a reference.
There was a problem hiding this comment.
Because base_table_identifier will be passed to TableCommit::builder later around L153. I think this is probably a little bit overengineering 😄 fixed
Which issue does this PR close?
Related Issue
Closes:
What changes are included in this PR?
This PR wraps up the effort to make Transaction API + TransactionAction retryable!
With
Transactionholds retryableactions, it no longer needs to hold staging variables likecurrent_table,updates, orrequirements. These can be generated withindo_commit.Are these changes tested?
Existing unit tests