snmvaughan commented on PR #6031: URL: https://github.com/apache/datafusion-comet/pull/6031#issuecomment-5818329965
Thanks both. The findings were right, and most of them came from one design choice: the store used for a request was decided by a mutable, process-lifetime latch inside the retry wrapper, and scopes were discovered one session at a time from 403s. I've replaced that design rather than patching it, in cb1bdec08. **New design.** The opt-in interface is now `CometS3LocationScopedCredentialProvider.getPolicyLocations(String bucket)`: the provider lists every location in the bucket that has its own policy, instead of reporting what one session covers. As andygrove suggested, the registered store is the selector: `LocationScopedObjectStore` is cached and registered once per `(bucket, config_hash, backend)`, holds a store per location, and routes each request by `location` to the longest covering location. The bucket root is an implicit location. How each finding is addressed: - **A/B/A and partitions spanning scopes (sunchao):** routing is per request with no mutable backing store. There's a test that reads three locations in rotating order through one store. - **Prefix format and segment boundary (sunchao):** locations and request paths are both canonicalized with `Path::from_url_path` and matched segment by segment, so `table` doesn't cover `table_extra`. Tests cover the contract's format, slash variants and `%`-escapes. - **Concurrent 403s (sunchao):** a 403 re-fetches the locations, then retries once if the route changed. The retry budget is per request, with no latch. Every refresh attempt starts a new snapshot generation, so requests routed from the same snapshot share one attempt, including a failed one. Tests cover two in-flight `get_opts` and `get_ranges` requests failing together, reads on separate threads waiting for a refresh in progress, and a shared failed refresh. - **Nested runtime (sunchao):** the region is resolved when the bucket's store is created. Location stores are built from that template and never `block_on`, which a test checks inside the runtime. - **Dedup (andygrove):** gone. Nothing is appended on recovery; recovery only re-reads the provider's list. - **JNI (andygrove):** the dispatcher returns a `String[]` copy. List code runs inside the checked `jni_static_call!`, non-`String` elements fail with `ArrayStoreException` in Java, and native frees each element's local ref. - **Registry invariant (andygrove, sunchao):** one store per key again, so the `register_object_store` comment holds as written. - **Base providers (sunchao, andygrove):** unaffected. The dispatcher returns `null` without calling them, and `create_store` builds the same store as before. The user guide now says exactly that. - **Versioning policy (andygrove):** the new type is listed, along with what adding a method or changing how paths match locations would mean. The policy also asks for the addition to be agreed in an issue first; I'll open one and link it from the PR. - **Docs (andygrove):** the user guide and design notes are rewritten against the new behavior, and Prettier passes. - **Integration tests (andygrove, sunchao):** a dedicated bucket with a per-bucket provider override gives the scoped tests their own cache key. They assert which credential path each read requested, with four locations read in one partition, rather than counting callbacks; the reuse test runs a warm-up scan first so it does not depend on test order. The suite needs Docker, and I haven't run it on this revision yet. The PR description is updated. Could a maintainer approve the workflow runs for this revision? Comet CI and CodeQL are waiting for approval. Happy to walk through any of it. -- 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]
