andygrove commented on code in PR #6478: URL: https://github.com/apache/datafusion-comet/pull/6478#discussion_r4149223175
########## docs/source/user-guide/latest/s3-credential-providers.md: ########## @@ -295,15 +297,15 @@ public final class MyLocationProvider implements CometS3LocationScopedCredential } ``` -Comet serves each request with the credential of the longest location that covers its path. A location covers a path when the path is the location itself or lies below it, compared one `/`-separated segment at a time, so `warehouse/sales` covers `warehouse/sales/part-0.parquet` but not `warehouse/sales_eu/part-0.parquet`. The bucket root covers every path that no returned location covers, and an empty list serves the whole bucket with the root's credential. Write locations the way `CometS3CredentialContext.getPath()` writes paths: percent-encoded, without the scheme or bucket name. A literal `%` must be written as `%25`; other characters may be left unencoded, and a leading or trailing `/` is optional. When several locations decode to the same path, Comet keeps the first. +Comet serves each request with the credential of the longest location that covers its path. A location covers a path when the path is the location itself or lies below it, compared one `/`-separated segment at a time, so `warehouse/sales` covers `warehouse/sales/part-0.parquet` but not `warehouse/sales_eu/part-0.parquet`. The bucket root covers every path that no returned location covers, and an empty list serves the whole bucket with the root's credential. Write locations the way `CometS3CredentialContext.getPath()` writes paths: percent-encoded, without the scheme or bucket name. A literal `%` must be written as `%25`; other characters may be left unencoded, and a leading or trailing `/` is optional. When several locations decode to the same path, Comet keeps the first. A literal `%` is common on the Iceberg path, where Comet compares a location with each file's key as Iceberg wrote it. Iceberg escapes partition values, so a location for the partition directory `ts=2024-01-01T00%3 A00` is written `ts=2024-01-01T00%253A00`. -Comet requests a location's credential by calling `getCredentialsForPath` with the location as the path, as you returned it but with a leading slash. Every request under a location shares that credential, so it must authorize every path the location is the longest match for, and your cache can key on the location. Locations apply to Comet's native Parquet reads only; Iceberg reads call `getCredentialsForPath` as they do for any provider. +Comet requests a location's credential by calling `getCredentialsForPath` with the location as the path, as you returned it but with a leading slash. Every request under a location shares that credential, so it must authorize every path the location is the longest match for, and your cache can key on the location. Locations apply to Comet's native Parquet reads and to its native Iceberg reads and writes. On the Iceberg path each data and delete file is routed by its own path, in its own bucket, so a table whose files span several locations or buckets gets each file's credential right. A table whose metadata location has no host is the exception; see [Enabling a bridge](#enabling-a-bridge). -**When Comet asks.** Comet calls `getPolicyLocations` when it creates the store for a bucket on an executor and keeps the answer for later reads of that bucket with the same S3 configuration. Reads that start at the same moment may each create a store and call it. If a read then fails with 403, or because `getCredentialsForPath` threw for the location Comet sent it to, Comet asks again, once for all the reads that failed on the same answer, and retries each read once if its path now falls under a different location. So a location added while a job runs is picked up even when you vend no credential for the bucket root, and a location you drop stops being used once its credential fails. A location added or removed without a read failing on it is not seen until the executor creates a new store. Make `getPolicyLocations` thread-safe and independent of where it runs; it may be called on the driver or on executors. +**When Comet asks.** Comet calls `getPolicyLocations` when it creates the store for a bucket on an executor and keeps the answer for later reads of that bucket with the same S3 configuration. On the Iceberg path it asks once per catalog and access mode on an executor, for the bucket of the first table's metadata or data location, and again when a table or file in another bucket is first used. Every table of the catalog shares the answers, which Comet keeps while it has one of the catalog's tables cached. Reads that start at the same moment may each create a store and call it. If a read then fails with 403, or because `getCredentialsForPath` threw for the location Comet sent it to, Comet asks again, once for all the reads that failed on the same answer, and retries each read once if its path now falls under a different location. On the Iceberg path writes and deletes do the same, except that a file being streamed is not sent again: its task fails, and Spark's retry of the task writes it with the new answer. So a location added while a job runs is picked up even when you vend no credential for the bucket root, and a location you drop stops being used once its credential fails. A location added or removed without a read failing on it is not seen until the executor creates a new store. Make `getPolicyLocations` thread-safe and independent of where it runs; it may be called on the driver or on executors. Review Comment: The shared locations are keyed by the whole property bag, and `CometScanRule` merges each table's FileIO properties from `LoadTableResponse` into it. A REST catalog that vends per-table credentials therefore gets its own registration, and its own `getPolicyLocations` calls, for each table. Could this sentence, and the matching one at line 144 of the design doc, say that tables share the answers when their catalog properties match? Vendors will use this paragraph to plan call volume. -- 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]
