andygrove opened a new pull request, #6545: URL: https://github.com/apache/datafusion-comet/pull/6545
Backport of #6509 to `branch-1.1`. Cherry-picked from `0bb1b32bab7aa8e3e4c8a4ee37e813c034273254`. Three files conflicted: `credential_bridge.rs`, `iceberg_common.rs` and the S3 credential provider design doc. All three conflicts come from #6106, the per-executor Iceberg `FileIO` cache, which is on `main` but not on `branch-1.1` (#6306 was closed). The adaptations: - `credential_bridge.rs`: none. The conflict was in a context line, the comment on the missing-expiry warning latch, which #6106 reworded on `main`. `branch-1.1` keeps its own wording, and the lines #6509 adds and removes match upstream. - `iceberg_common.rs`: dropped. #6509 only reworded the comment on #6106's `FILE_IO_CACHE`, which `branch-1.1` doesn't have. - Design doc: #6509 also rewrote a paragraph in #6106's "Executor `FileIO` cache on the Iceberg path" section. `branch-1.1` has no such section, so that paragraph is dropped. The rest of the doc change matches upstream. - `CometS3CredentialBridgeSuite` and the user guide, described next. Without #6106, the Iceberg half of #6509 does less on `branch-1.1` than on `main`. `branch-1.1` builds a `FileIO`, and with it a credential bridge, for each task, so a credential is reused only within the task that fetched it. Within a task the provider is asked once instead of once per storage call, but each task asks again. On `main` the bridge lives in the executor's `FileIO` cache, so reads after the first don't ask at all. The Parquet half is unaffected: its bridges are executor-wide through the object store cache, as on `main`. That changes two files: - **The new Iceberg test.** Upstream's version asserts that two reads after a warm-up read don't call the provider at all. On `branch-1.1` that fails with 4 calls. The adapted test drops the warm-up read and the `ORDER BY`, whose range partitioning runs the scan a second time to sample it, and asserts one call per read. Each read is one task over the table's three data files. - **The user guide**, which is published from this branch. After "Comet makes at most one call at a time for each location ..." it adds: "On the Iceberg path, the reuse and the sharing happen within a task, so each task asks you at least once." ## Which issue does this PR close? Closes #6508 on `branch-1.1`. #6509 already closed it on `main`. ## Rationale for this change #6509 merged after `branch-1.1` was cut at 36ab57c68 and is labeled `backport-1.1`. Without it, 1.1.0 calls a credential provider once per HTTP request on the Parquet path and once per storage call on the Iceberg path, however long the credential lasts. A Parquet request retried after backoff can carry a signature whose session token has expired, and `object_store` doesn't retry a 403. A provider that reports `Long.MAX_VALUE`, or an expiry in seconds, fails every Iceberg request. Not proposed for `branch-1.0`: #6509 has no `backport-1.0` label. #6478, also labeled `backport-1.1`, adds `CometS3CredentialBridge::for_location`, which on `main` has to set the `cache` field this PR adds. Its backport therefore needs this one first. ## What changes are included in this PR? The original change plus the adaptations above; see #6509 for the details. In short: - `credential_bridge.rs`: each bridge keeps its last credential with a known expiry and reuses it until 5 minutes before that expiry. It makes at most one provider call at a time, and requests that waited for that call share its outcome. `0` (unknown) and `Long.MAX_VALUE` (no expiry) are not kept, and an expiry before 2000 is treated as unknown, with a warning. - JVM side: the `IcebergRESTVendedS3Provider` example reports the vended expiry instead of `0`, and the `CometS3Credentials` Javadoc and the Spark 3.x adapter's comment describe the expiry semantics. - The user guide and the design doc replace the "no Comet-side cache" rule with the expiry-bounded reuse. - Tests: six Rust unit tests, a Parquet and an Iceberg test in the MinIO suite, and the expiry assertion in `IcebergRESTVendedS3ProviderTest`. ## How are these changes tested? Run locally on this branch with JDK 17: - Rust: the 36 `cloud::s3` tests pass, including the six new `credential_bridge` tests. `cargo check --all-targets`, `cargo fmt --check` and workspace clippy with `--all-targets -D warnings` are clean. - JUnit tests in `org.apache.comet.cloud.s3`: 47 pass on Spark 4.1 and 40 on Spark 3.5. - `CometS3CredentialBridgeSuite`, which CI does not run because it needs Docker, ran against S3Proxy in place of MinIO. On Spark 4.1 all 8 tests pass. On Spark 3.5, 7 of 8 pass, including both new tests. The failure is the REST catalog test: S3Proxy rejects Spark's own Iceberg write, before any Comet code runs, with an `x-amz-content-sha256` mismatch, as it did when #6318 was tested. - Without the fix: with `credential_bridge.rs` reverted to `branch-1.1`'s and the tests kept, both new suite tests fail on Spark 4.1. The Parquet test sees 20 provider calls where it expects 0, and the Iceberg test 6 where it expects 2. - Spotless, scalastyle and `prettier --check` on the two docs pass. Against `branch-1.1`, the changed paths route this pull request to every suite except Spark 3.4's SQL job, the PyArrow UDF job and the benchmark check. -- 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]
