andygrove opened a new pull request, #6226:
URL: https://github.com/apache/datafusion-comet/pull/6226
Backport of #6031 to `branch-1.0`.
Cherry-picked from `e5d0b7575f1b7bfe4b19da7246f2d9733357e18f`. Fifteen of
the sixteen files applied cleanly, and their changes match upstream's apart
from line offsets. `native/core/src/parquet/objectstore/s3.rs` had one
conflict, over an error message. It is described under "What changes are
included" below.
## Which issue does this PR close?
Closes #6207 on `branch-1.0`. #6031 already closed it on `main`.
## Rationale for this change
The limitation that #6031 removes ships in 1.0.0. On the Parquet path,
`object_store::CredentialProvider::get_credential()` receives no request path,
so a `CometS3CredentialProvider` gets one credential per bucket. Comet requests
it with the path of the first file it reads. A vendor whose policies differ by
location within a bucket, for example `warehouse/sales` and
`warehouse/finance`, gets 403s on every location but the first.
On `branch-1.0`, the SPI classes, the JNI dispatcher and the credential
bridge match `main` just before #6031, apart from an `#[allow(dead_code)]` on
`AccessMode::Write`. `create_store` differs only in the error message below.
The change is opt-in. Providers that implement only
`CometS3CredentialProvider` get the same store as before. The only addition is
one dispatcher call when the store cache misses. It returns `null` without
reaching the provider.
## What changes are included in this PR?
The change is the original one, so see #6031 for the details. It adds the
`@Public` `CometS3LocationScopedCredentialProvider`,
`CometS3CredentialDispatcher.getPolicyLocations` and the native
`LocationScopedObjectStore`. It also updates the user guide, the design notes
and the versioning policy's list of SPI classes.
One adaptation:
- In `s3.rs`, `S3StoreTemplate::new` keeps `branch-1.0`'s region-lookup
error, `Failed to resolve region: {e}`. #6031 moved that code into the new
template. On `main`, #5314 had already extended the message with advice for
S3-compatible services. #5314 isn't on `branch-1.0`, so the message stays as it
is here. Apart from the message, the file matches `main`'s copy after #6031.
#6212 affects this code on `branch-1.0` as well. The store fetches the
locations again only after a 403. A provider with no policy for the path it is
asked about usually throws instead, and a throw does not trigger the refresh.
So reads under a location that was added or dropped mid-job keep failing until
the executor restarts. The fix is #6223, which is open on `main` and also
labelled `backport-1.0`. It would go on top of this PR. It also corrects the
user guide's "When Comet asks" paragraph, which this PR carries unchanged.
## How are these changes tested?
The original PR's tests, run locally on `branch-1.0`:
- Rust: all 16 `location_scoped` tests pass, as does the new
`builds_from_a_template_inside_the_runtime` test in `s3.rs`. The rest of
`parquet::objectstore` passes too, 65 tests in all. `cargo fmt --all --
--check` and `cargo clippy --all-targets --workspace -- -D warnings` pass on
rustc 1.97.
- JVM, on the branch's default profile (Spark 4.1.3, Scala 2.13, JDK 17):
the new `CometS3LocationScopedCredentialProviderTest` (9 tests) passes. So do
the existing `CometS3CredentialDispatcherTest` (16 tests) and
`CometPublicApiSuite`, which now pins the new type.
- The Lint Java job's command for Spark 3.4, Scala 2.12 and JDK 11 passes:
`package -DskipTests scalafix:scalafix -Dscalafix.mode=CHECK -Psemanticdb`. It
compiles the test sources on Scala 2.12, the build that #6031's `Set.of` fix
was for. Spotless and scalastyle pass, and so does Prettier on the three
changed docs.
`CometS3CredentialBridgeSuite`, the MinIO end-to-end suite, was not run
because it needs Docker. CI doesn't run it on either branch.
## Are there any user-facing changes?
For S3 credential provider vendors, yes. `branch-1.0` gains the `@Public`
`CometS3LocationScopedCredentialProvider` interface, and the versioning
policy's list of SPI classes names it. Providers that don't implement it see no
change. 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]
