-
Notifications
You must be signed in to change notification settings - Fork 266
feat(WIP): Add TransactionAction #1400
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
base: main
Are you sure you want to change the base?
Conversation
crates/catalog/memory/src/catalog.rs
Outdated
Err(Error::new( | ||
ErrorKind::FeatureUnsupported, | ||
"MemoryCatalog does not currently support updating tables.", | ||
)) | ||
} | ||
|
||
async fn commit_table(&self, base: &Table, current: Table) -> Result<Table> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm trying to replace update_table
with commit_table
, also would love some feedback on this:
- Do we actually need
TableCommit
for catalog to commit tables?- In the
Transaction
we have already staged changes to thecurrent_table
link. Transaction
will need to holdVec<TransactionAction>
so tx itself can rebase updates easily, if needed.- Catalogs will need to check if metadata location match and retry based on the check result,
TableCommit
may be useful in this case, but the logic to apply updates on the base table is still duplicate toTransaction::apply()
(https://github.com/apache/iceberg-rust/blob/main/crates/iceberg/src/transaction/mod.rs#L70)
- In the
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some suggestions.
self | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pub struct SetLocationAction {
tx: Option<Transaction>
location: Option<String>
}
impl SetLocation {
fn apply(self) -> Result<Transaction> {
let tx = self.tx.take();
self.tx.add_action(Arc::new(self));
}
}
Co-authored-by: Renjie Liu <liurenjie2008@gmail.com>
Co-authored-by: Renjie Liu <liurenjie2008@gmail.com>
// catalog.clone()).await | ||
// } | ||
// }).await | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried both tokio-retry2 and backon.
Both will require us toss around an intermediate Error type (RetryError
for tokio-retry2 and anyhow::Error
for backon). backon
integrates better with my existing code, which requires &mut self
in the do_commit
method. Also, somewhat surprisingly, tokio-retry2
doesn't provide min_delay
.
I've chosen to go with backon
here but would appreciate any other feedback
Which issue does this PR close?
Related Issues:
Transaction::commit
method #1387TableMetadata
to new location. #1388What changes are included in this PR?
Are these changes tested?