feat(transaction): Add retry logic to transaction #1484
Conversation
Hi @dentiny , this is a good question:) I'm thinking of using a mock catalog, but haven't figured out exactly how |
| .expect("Invalid value for commit.retry.num-retries"), | ||
| ) | ||
| .with_factor(2.0) | ||
| .build() |
There was a problem hiding this comment.
Not really related to this change, but something I noticed while writing the PR:
- There is not a centralized properties class, which makes properties hard to locate in the code base: Some properties are defined in table_metadata.rs, and some are defined in the classes that use them: example
- No orthogonal way to fetch and parse property value, which makes error handling inconsistent. For example, this LOC will silently fallback to the default value if property parsing failed
For 1, we can have a dedicated file like table_metadata.rs to hold all hardcoded property keys, or we can have a property struct to define properties we support like what datafusion does
For 2, something like iceberg-java's PropertyUtil can be very useful
There was a problem hiding this comment.
I'm in favor of 1. I agree that we could have a module like table_properties to contain all the properties, and have sth to parse properties in a type safe way like following:
pub struct TableProperties {
#[key="commit.num.retries", default="1"]
commitRetries: i32
}
We could track this in following up issue.
There was a problem hiding this comment.
I've created the issue to track the task of centralizing properties: #1505
|
cc @liurenjie1024 @Xuanwo this PR is ready for review, PTAL, thanks! |
| .expect("Invalid value for commit.retry.num-retries"), | ||
| ) | ||
| .with_factor(2.0) | ||
| .build() |
There was a problem hiding this comment.
I'm in favor of 1. I agree that we could have a module like table_properties to contain all the properties, and have sth to parse properties in a type safe way like following:
pub struct TableProperties {
#[key="commit.num.retries", default="1"]
commitRetries: i32
}
We could track this in following up issue.
| @@ -82,6 +83,7 @@ itertools = "0.13" | |||
| linkedbytes = "0.1.8" | |||
| metainfo = "0.7.14" | |||
| mimalloc = "0.1.46" | |||
| mockall = "0.13.1" | |||
There was a problem hiding this comment.
Should we add it to dev dependency? Since it's only used in tests
There was a problem hiding this comment.
Yes, I saw you created a PR already: #1507
Thanks for catching this!
## What changes are included in this PR? `mockall` dependency is introduced in #1484, which is used for unit test on retry. But it's only used in unit tests, but not production code; so it should be placed under dev section, and completely disabled in production code path. Current implementation unnecessarily increases crate size for production usage. ## Are these changes tested? N/A, a no-op change, a passed compilation should be enough
Which issue does this PR close?
Transaction::commitmethod #1387What changes are included in this PR?
backonAre these changes tested?
Added unit tests