andygrove opened a new issue, #6221: URL: https://github.com/apache/datafusion-comet/issues/6221
#6031 adds `CometS3LocationScopedCredentialProvider`. With it, the bucket's `LocationScopedObjectStore` fetches the provider's location list again only when a request fails with `PermissionDenied`, which is a 403 from S3. When the provider throws from `getCredentialsForPath` instead, the bridge turns the exception into `object_store::Error::Generic`, so the store keeps routing with its old list. The store is cached for the life of the executor, and the object store cache has no eviction. So those reads keep failing until the executors restart. Two cases hit this: - A location is added while the executors are running, and the provider has no bucket-wide credential. Reads under the new location route to the bucket root `/`, the provider throws for `/`, and the new location is never picked up. - The provider drops a location or folds it into a broader one, and throws for the old location. Reads under it keep going to the old location and failing, while a freshly built store would serve them from the broader one. I reproduced both at `13e634463` with a fake location store in `location_scoped.rs` that returns the provider's error. Each case fetched the locations zero times across three failing reads, while a fresh store served the same path. The review thread has the details: https://github.com/apache/datafusion-comet/pull/6031#discussion_r4098072041 The failure is loud. Comet never returns wrong results or falls back to a broader credential. A one-off provider error, such as an STS throttle, does not stick either, because the provider is called on every request. Only the stale routing persists. It affects only providers that implement the new interface. It is most likely on long-lived executors, such as a Thrift server, Spark Connect or a streaming job, where new locations appear while the executors are up. A provider with no bucket-wide credential is the natural shape for per-location policies, and the user guide's "return credentials or throw" advice points vendors toward throwing. Restarting the executors works around it, and so does returning a credential that S3 rejects instead of throwing, but neither is documented. One fix that stays inside Comet is for the bridge to wrap its credential-fetch error in a typed source, and for `location_scoped.rs` to treat a `Generic` error carrying it like `PermissionDenied`, with the same bounded refresh and one retry when the route changes. `object_store` passes provider errors through unchanged, so the store can see it. That changes nothing in the `@Public` API. The alternative is to require `getCredentialsForPath` to answer for `/` and for dropped locations. That changes the contract of a public interface, so it would have to be settled before a release ships it. Either way, the "When Comet asks" paragraph in `s3-credential-providers.md` should say that a provider throwing is not a 403. This should land in the same release that ships `CometS3LocationScopedCredentialProvider`. Otherwise the guide's claim that a location added while a job runs is picked up does not hold for a provider without a bucket-wide credential. -- 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]
