xanderbailey commented on PR #3605: URL: https://github.com/apache/iceberg-rust/pull/3605#issuecomment-6686426550
Thanks for picking up the layering so quickly! The precedence behaviour is what I was after, but I think I explained the placement badly: I didn't mean for the `s3.retry.*` keys to live on `OpenDalClientConfig` alongside the `opendal.*` ones. `OpenDalClientConfig` is documented as the backend-independent settings shared by every variant, and with the S3 fields on it every backend now parses (and validates) `s3.retry.*`, and they get serialized into `client_config` for GCS/Azure/memory etc. too. What I had in mind: - `OpenDalClientConfig` only knows about `opendal.retry.*` (plus `opendal.io-timeout-ms`). - The `s3.retry.*` keys are parsed in `s3.rs`, next to the rest of the `s3.*` properties that `s3_config_parse` already handles. - The S3 arm of `OpenDalStorageFactory::build` overlays those S3 values onto the client config per key, so the S3 variant's `client_config` holds the resolved policy. Then `retry_layer` doesn't need to know which backend it's building for, the `OpenDalStorage::retry_layer` dispatch and `s3_retry_layer` go away, and adding GCS/Azure-specific overrides later follows the same pattern without growing the shared struct. Also, the title and description still say only S3 applies retry settings and the other backends keep OpenDAL's default, which isn't true anymore with `opendal.retry.*`. Could you update them? -- 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]
