Add credentials refresh in more locations #165
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Collaborator
|
@J-Meyers: Thanks for the PR, just an heads up that it might take a bit for this to get properly through review process, but we'll get to it. Testing is indeed not trivial, unsure what's the way forward, but I'll think of something |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is very difficult to test since you must wait for the credentials to timeout, however there were a variety of locations and scenarios that previously did not automatically refresh the credentials.
After the first
S3FileHandlewasInitialized then many operations like GLOBs would never have an opportunity to refresh the handle.This adds in autorefresh around all calls, the core of this is the
ExecuteWithRefreshand keeping around theFileOpener.ExecuteWithRefreshis used to just wrap the old code exactly as it was with a mechanism to automatically refresh the credentials and then run it again.The previous refreshing in
S3FileHandle::Initializeis now removed since it is now managed at each of the lower level function calls.Should close:
duckdb/duckdb-aws#26
duckdb/duckdb-aws#64
duckdb/duckdb-aws#97
If you have any advice on how to create an automated test for this I'd appreciate it