REST: introduce FileIOBuilder to pass storage credentials at construction time#16536
REST: introduce FileIOBuilder to pass storage credentials at construction time#16536findinpath wants to merge 1 commit into
Conversation
…tion time The existing BiFunction<SessionContext, Map<String, String>, FileIO> ioBuilder applied storage credentials post-construction via SupportsStorageCredential.setCredentials method which does not work for engines (e.g. Trino) that need all inputs available at build time. Introduce a FileIOBuilder functional interface that receives SessionContext, fileIO properties, and List<StorageCredential> all at once. Deprecate the BiFunction constructor with a wrapper that preserves the old two-step behavior for existing callers.
gaborkaszab
left a comment
gaborkaszab
left a comment
There was a problem hiding this comment.
Thanks for the PR, @findinpath ! Just tried to understand the issue, asked some clarification.
| } | ||
|
|
||
| /** | ||
| * @deprecated Use {@link #RESTSessionCatalog(Function, FileIOBuilder)} instead. |
There was a problem hiding this comment.
Please mention which is the target release for removal. Technically, in core/ we can drop one minor release after deprecation. deprecation now is released in 1.12.0, we can drop in 1.13.0. Unless we say this is considered public API even though in core/
| : (context, properties, credentials) -> { | ||
| FileIO fileIO = ioBuilder.apply(context, properties); | ||
| if (!credentials.isEmpty() | ||
| && fileIO instanceof SupportsStorageCredentials ioWithCredentials) { |
There was a problem hiding this comment.
Is the issue that the ioBuilder passed by Trino returns a FileIO that is not an instance of SupportsStorageCredentials?
Isn't it the "contract" for FileIO that storage credentials can be set through SupportsStorageCredentials.setCredentials()? How difficult would it be to adjust the Trino side to work with this design?
2 participants
Problem
The existing
BiFunction<SessionContext, Map<String, String>, FileIO> ioBuilderapplied storage credentials post-construction viaSupportsStorageCredential.setCredentialsmethodwhich does not work for engines (e.g. Trino) that need all inputs available at build time.
Technical solution
Introduce a
FileIOBuilderfunctional interface that receivesSessionContext, fileIO properties, andList<StorageCredential>all at once. Deprecate theBiFunctionconstructor with a wrapper that preserves the old two-step behavior for existing callers.Related issues and PRs
This contribution is attempting to adjust the previous changes performed in #15752 for configuring
StorageCredentialsin theioBuilder.Issue discovered while developing trinodb/trino#29590
The Trino PR makes use of this contribution for ensuring the accuracy of these changes on https://github.com/trinodb/trino project within the Iceberg connector.