andygrove opened a new pull request, #6276: URL: https://github.com/apache/datafusion-comet/pull/6276
Backport of #6223 to `branch-1.0`. Cherry-picked from `68c4e9b72d639c37d87eb0f08a111c2f8de708d9` without conflicts. No adaptations were needed. ## Which issue does this PR close? Closes #6212 and #6221 on `branch-1.0`. ## Rationale for this change #6226 brought the location-scoped S3 credential SPI (#6031) to `branch-1.0` after 1.0.0, so 1.0.1 would be its first release. Without this PR it would ship with the bug #6223 fixed on `main`. `location_scoped.rs` on `branch-1.0` is identical to `main`'s copy just before #6223. A `LocationScopedObjectStore` fetches the provider's policy locations again only after a 403. A provider with no policy for a path throws instead, and the bridge reports that as a plain `Generic` error. So a location added while the executors are up is never picked up when the provider vends no credential for the bucket root. A location the provider has dropped also keeps being used after it stops vending it. Both stay broken until the executors restart. ## What changes are included in this PR? The fix is the original one; see #6223 for the details. The bridge gives its failures a `CredentialProviderError` source, which `object_store` passes through to the read unchanged. The store treats a read that fails with one like a 403: the same bounded refresh, and one retry if the path now routes to a different location. Other errors still return without a refresh. The S3 credential provider user guide and design doc describe the new behaviour. ## How are these changes tested? Same tests as the original PR, run locally on `branch-1.0`: - All 21 `location_scoped` tests pass, including the five new ones, and so do the other 49 tests under `parquet::objectstore` and `cloud`. - The bug is present on `branch-1.0`, and the tests catch it. With `may_mean_stale_locations` narrowed back to 403s only and the tests kept, four of the five new tests fail. The fifth, `does_not_refresh_on_other_errors`, passes either way, as it should. - `cargo fmt --all -- --check`, `cargo clippy --all-targets --workspace -- -D warnings` and `prettier --check` on the two changed docs pass. `CometS3CredentialBridgeSuite` exercises the bridge against MinIO in Docker and was not run locally. ## Are there any user-facing changes? Only for providers that implement `CometS3LocationScopedCredentialProvider`, which #6226 adds on `branch-1.0`. A read whose credential the provider cannot produce now fetches the locations again, as a 403 already did. So locations added or removed while the executors are up take effect without a restart. There are no config changes. -- 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]
