xanderbailey commented on code in PR #3605:
URL: https://github.com/apache/iceberg-rust/pull/3605#discussion_r4493068664
##########
crates/storage/opendal/src/lib.rs:
##########
@@ -146,9 +198,46 @@ impl OpenDalClientConfig {
pub(crate) fn io_timeout(&self) -> Duration {
Duration::from_millis(self.io_timeout_ms.get())
}
+
+ /// The retry policy of an S3 storage: see [`Self::retry_layer`].
+ pub(crate) fn s3_retry_layer(&self) -> RetryLayer {
+ self.retry_layer(true)
+ }
+
+ /// The retry policy, with jitter; OpenDAL's default when no key is set.
Each setting is resolved on its own:
+ /// the `s3.retry.*` key (S3 only), then the `opendal.retry.*` key, then
OpenDAL's default.
+ pub(crate) fn retry_layer(&self, s3: bool) -> RetryLayer {
Review Comment:
If the S3 overrides are resolved into the client config when the S3 storage
is built, this can drop the `s3: bool` flag and just build the layer from
`opendal.retry.*` fields, which by then already hold the right values for each
backend.
##########
crates/storage/opendal/src/lib.rs:
##########
@@ -129,15 +141,55 @@ pub struct OpenDalClientConfig {
#[property(
key = OPENDAL_IO_TIMEOUT_MS,
default = DEFAULT_IO_TIMEOUT_MS,
- parse_with = parse_io_timeout_ms
+ parse_with = parse_positive_ms
)]
io_timeout_ms: NonZeroU64,
+ /// Retries on any backend.
+ #[property(key = OPENDAL_RETRY_NUM_RETRIES, default = None)]
+ retry_num_retries: Option<usize>,
+ /// Delay before the first retry on any backend in milliseconds.
+ #[property(
+ key = OPENDAL_RETRY_MIN_WAIT_MS,
+ default = None,
+ parse_with = parse_positive_ms
+ )]
+ retry_min_wait_ms: Option<NonZeroU64>,
+ /// Upper bound of the delay between two retries on any backend in
milliseconds.
+ #[property(
+ key = OPENDAL_RETRY_MAX_WAIT_MS,
+ default = None,
+ parse_with = parse_positive_ms
+ )]
+ retry_max_wait_ms: Option<NonZeroU64>,
+ /// S3 retries.
+ #[property(key = S3_RETRY_NUM_RETRIES, default = None)]
+ s3_retry_num_retries: Option<usize>,
+ /// S3 delay before the first retry in milliseconds.
+ #[property(
+ key = S3_RETRY_MIN_WAIT_MS,
+ default = None,
+ parse_with = parse_positive_ms
+ )]
+ s3_retry_min_wait_ms: Option<NonZeroU64>,
+ /// S3 upper bound of the delay between two retries in milliseconds.
+ #[property(
+ key = S3_RETRY_MAX_WAIT_MS,
+ default = None,
+ parse_with = parse_positive_ms
+ )]
+ s3_retry_max_wait_ms: Option<NonZeroU64>,
Review Comment:
These are S3-specific so I don't think they belong on `OpenDalClientConfig`
##########
crates/storage/opendal/src/lib.rs:
##########
@@ -507,10 +596,19 @@ impl OpenDalStorage {
// benefits non-object-store backends.
let operator = operator
.layer(TimeoutLayer::new().with_io_timeout(self.client_config().io_timeout()))
- .layer(RetryLayer::new());
+ .layer(self.retry_layer());
Ok((operator, relative_path))
}
+ /// Only S3 reads the `s3.retry.*` properties; the other backends read
`opendal.retry.*`.
+ fn retry_layer(&self) -> RetryLayer {
Review Comment:
With the overrides resolved at build time this per-variant match isn't
needed; `create_operator` can call `self.client_config().retry_layer()` the
same way it does for `io_timeout()`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
