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

fix: only try to create storage client if storage is enabled #9913

Merged
merged 2 commits into from
May 23, 2022

Conversation

pauldambra
Copy link
Member

@pauldambra pauldambra commented May 23, 2022

Problem

In this Slack thread @macobo reported that without object storage configured there were frequent unhelpful log messages and requests were delayed by the healthcheck.

The object_storage.py module did not check if storage was enabled before trying to create an s3 client

Changes

  • does not try to create an s3 client if object storage is not enabled
  • returns False from the health check if object storage is not enabled without checking for bucket access
  • aggressively reduces timeout for S3 client so we don't need to wait 5 seconds on a service with very high availability and low latency

Note

How did you test this code?

ran it locally and added a test to clarify behaviour

@pauldambra pauldambra marked this pull request as ready for review May 23, 2022 14:42
Copy link
Contributor

@hazzadous hazzadous left a comment

Choose a reason for hiding this comment

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

LGTM to resolve issues people may be having

I gather now we'd want anywhere that calls this to check if None was returned?

@pauldambra pauldambra merged commit d3dd249 into master May 23, 2022
@pauldambra pauldambra deleted the calm_down_object_storage branch May 23, 2022 15:31
@pauldambra
Copy link
Member Author

Yep, possibly better to have a null object

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