Spec: Add expiry time config to REST table load #10873
Conversation
|
FYI there is also #10722 to standardize the different credentials |
|
Thank you @nastra, for sharing the PR. The inspiration for the current PR is both #10722 and #10576 (asked to add expiry property in separate PR). As mentioned #10722, the preference would be given to config then credential for backward compatibility. |
dennishuo
left a comment
dennishuo
left a comment
There was a problem hiding this comment.
+1 to having this here in config even if it will eventually be moved into the new "Credentials" field per https://github.com/apache/iceberg/pull/10722/files
During the transitional period as clients get updated to use the new Credentials tuple, the lower-layer plumbing for clients learning to do something smart with an expiry time can be unblocked, and the interface at which the client library plumbs REST-response into expected fields can be abstracted out to switch from direct config keys to the Credentials tuple all at once, instead of having core aspects of plumbing that can only be plugged in later.
open-api/rest-catalog-open-api.yaml
Outdated
| - `token`: Authorization bearer token to use for table requests if OAuth2 security is enabled | ||
| - `token`: Authorization bearer token to use for table requests if OAuth2 security is enabled | ||
| - `expires-at-ms`: if present, specifies timestamp in milliseconds when credentials for storage(AWS S3/Azure/), specified in config would expire |
There was a problem hiding this comment.
Please specify the reference point in time for expires-at-ms. A number of milliseconds by themselves are not sufficient to deduce the expiry moment.
There was a problem hiding this comment.
I'd also suggest using ISO 8601 for the value format.
Co-authored-by: Alexandre Dutra <adutra@users.noreply.github.com>
| ## General Configurations | ||
| - `token`: Authorization bearer token to use for table requests if OAuth2 security is enabled | ||
| - `expires-at-ms`: if present, specifies timestamp in milliseconds when credentials for storage(AWS S3/Azure/GCP), specified in config would expire |
There was a problem hiding this comment.
I don't think it's actually correct to have this at this level. This should be specific to the underlying Storage provider. GCS for example has gcs.oauth2.token-expires-at. That being said, the respective FileIO properties of each storage provider should have a similar property to the one used for GCS
There was a problem hiding this comment.
Based on #10722 (comment), I was thinking to move/add this to fileIo Properties once linked PRs in the description are merged
I saw this comment, where it was decided not having the separate property.
It would be beneficial to have this property but will yet to go through the recording of Catalog sync (recording is not yet available)
There was a problem hiding this comment.
Yes, the idea is to move the expiration setting to the respective FileIO implementation rather than having a separate field in the OpenAPI spec
|
Closing this as Store specific expiry time config is being introduced cc @nastra |
5 participants
Add expiry time to table load, this is split from PR #10576.
Uses the same TimeUnit as #10722 for better alignment
FYI @nastra @Fokko
cc @stirupati @dennishuo