Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

error when s3 client is missing region config #330

Merged
merged 2 commits into from
Feb 27, 2023

Conversation

psFried
Copy link
Contributor

@psFried psFried commented Jan 27, 2023

When the region configuration is missing the AWS S3 client will fail essentially all operations. This is true even if the endpoint is provided in the storage URL. Because the client is cached, this same failure will continue to happen, even after you update ~/.aws/config to set the region.

This commit adds an explicit check for the Region when initializing a new S3 client, to prevent the invalid client configuration from being persisted in the clients map.

This is not urgent or blocking, but I just wanted to get the fix in while I was thinking about it.


This change is Reviewable

When the `region` configuration is missing the AWS S3 client
will fail essentially all operations. This is true even if the
`endpoint` is provided in the storage URL. Because the client is cached,
this same failure will continue to happen, even _after_ you update
`~/.aws/config` to set the `region`.

This commit adds an explicit check for the `Region` when initializing a
new S3 client, to prevent the invalid client configuration from being
persisted in the `clients` map.
#329 introduced a change to synchronously fail shards if the result of
`store.StartCommit` is immediately available. This is _technically_ a
race condition, since some asynchronous operations may still completed
and observed by `txnStartCommit`. This changes the test to only match
against a portion of the final error message, so that it tolerates the
error being returned either synchronously or asyncronously.
Copy link
Contributor

@jgraettinger jgraettinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@psFried psFried merged commit 747d3b6 into master Feb 27, 2023
@psFried psFried deleted the phil/handle-missing-region branch February 27, 2023 20:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants