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: improve new minio implementation #546

Merged
merged 3 commits into from
Mar 3, 2025
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
feat: remove usage of use_minio feature flag
this rollout is done and we are 100% enabled on prod so we are safe to
remove this and simplify the logic

starting off by removing all references to use_minio in shared
  • Loading branch information
joseph-sentry committed Feb 28, 2025
commit ce528d347980556655a127fc27d495317e1e192b
1 change: 0 additions & 1 deletion shared/rollouts/features.py
Original file line number Diff line number Diff line change
@@ -3,4 +3,3 @@
BUNDLE_THRESHOLD_FLAG = Feature("bundle_threshold_flag")
INCLUDE_GITHUB_COMMENT_ACTIONS_BY_OWNER = Feature("include_github_comment_actions")
USE_NEW_MINIO = Feature("use_new_minio")
USE_MINIO = Feature("use_minio")
35 changes: 9 additions & 26 deletions shared/storage/__init__.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
from typing import Literal

from shared.config import get_config
from shared.rollouts.features import USE_MINIO, USE_NEW_MINIO
from shared.rollouts.features import USE_NEW_MINIO
from shared.storage.aws import AWSStorageService
from shared.storage.base import BaseStorageService
from shared.storage.fallback import StorageWithFallbackService
@@ -14,16 +16,9 @@ def get_appropriate_storage_service(
repoid: int | None = None,
force_minio=False,
) -> BaseStorageService:
chosen_storage: str = get_config("services", "chosen_storage", default="minio") # type: ignore
if force_minio:
chosen_storage = "minio"

if repoid:
if USE_MINIO.check_value(repoid, default=False):
chosen_storage = "minio"

if USE_NEW_MINIO.check_value(repoid, default=False):
chosen_storage = "new_minio"
chosen_storage = "minio"
if repoid and USE_NEW_MINIO.check_value(repoid, default=False):
chosen_storage = "new_minio"

if chosen_storage not in _storage_service_cache:
_storage_service_cache[chosen_storage] = (
@@ -34,23 +29,11 @@ def get_appropriate_storage_service(


def _get_appropriate_storage_service_given_storage(
chosen_storage: str,
chosen_storage: Literal["minio", "new_minio"],
) -> BaseStorageService:
if chosen_storage == "gcp":
gcp_config = get_config("services", "gcp", default={})
return GCPStorageService(gcp_config)
elif chosen_storage == "aws":
aws_config = get_config("services", "aws", default={})
return AWSStorageService(aws_config)
elif chosen_storage == "gcp_with_fallback":
gcp_config = get_config("services", "gcp", default={})
gcp_service = GCPStorageService(gcp_config)
aws_config = get_config("services", "aws", default={})
aws_service = AWSStorageService(aws_config)
return StorageWithFallbackService(gcp_service, aws_service)
elif chosen_storage == "new_minio":
if chosen_storage == "new_minio":
minio_config = get_config("services", "minio", default={})
return NewMinioStorageService(minio_config)
else:
elif chosen_storage == "minio":
minio_config = get_config("services", "minio", default={})
return MinioStorageService(minio_config)
27 changes: 13 additions & 14 deletions shared/storage/new_minio.py
Original file line number Diff line number Diff line change
@@ -260,20 +260,19 @@ def read_file(
f"File {path} does not exist in {bucket_name}"
)
raise e
if response.headers:
content_encoding = response.headers.get("Content-Encoding", None)
if not self.zstd_default and content_encoding == "zstd":
# we have to manually decompress zstandard compressed data
cctx = zstandard.ZstdDecompressor()
# if the object passed to this has a read method then that's
# all this object will ever need, since it will just call read
# and get the bytes object resulting from it then compress that
# HTTPResponse
reader = cctx.stream_reader(cast(IO[bytes], response))
else:
reader = response
else:
reader = response
reader = response
if (
response.headers
and not self.zstd_default
and response.headers.get("Content-Encoding") == "zstd"
):
# we have to manually decompress zstandard compressed data
cctx = zstandard.ZstdDecompressor()
# if the object passed to this has a read method then that's
# all this object will ever need, since it will just call read
# and get the bytes object resulting from it then compress that
# HTTPResponse
reader = cctx.stream_reader(cast(IO[bytes], response))

if file_obj:
file_obj.seek(0)
36 changes: 1 addition & 35 deletions tests/unit/storage/test_init.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
from shared.rollouts.features import USE_MINIO, USE_NEW_MINIO
from shared.rollouts.features import USE_NEW_MINIO
from shared.storage import get_appropriate_storage_service
from shared.storage.aws import AWSStorageService
from shared.storage.fallback import StorageWithFallbackService
@@ -123,37 +123,6 @@ def test_get_appropriate_storage_service_new_minio(
"minio": minio_config,
}
mocker.patch.object(USE_NEW_MINIO, "check_value", return_value=True)
mocker.patch.object(USE_MINIO, "check_value", return_value=False)
res = get_appropriate_storage_service(repoid=123)
assert isinstance(res, NewMinioStorageService)
assert res.minio_config == minio_config

def test_get_appropriate_storage_service_use_minio(
self, mock_configuration, mocker
):
mock_configuration.params["services"] = {
"chosen_storage": "minio",
"gcp": gcp_config,
"aws": aws_config,
"minio": minio_config,
}
mocker.patch.object(USE_MINIO, "check_value", return_value=True)
mocker.patch.object(USE_NEW_MINIO, "check_value", return_value=False)
res = get_appropriate_storage_service(repoid=123)
assert isinstance(res, MinioStorageService)
assert res.minio_config == minio_config

def test_get_appropriate_storage_service_use_minio_and_new_minio(
self, mock_configuration, mocker
):
mock_configuration.params["services"] = {
"chosen_storage": "minio",
"gcp": gcp_config,
"aws": aws_config,
"minio": minio_config,
}
mocker.patch.object(USE_MINIO, "check_value", return_value=True)
mocker.patch.object(USE_NEW_MINIO, "check_value", return_value=True)
res = get_appropriate_storage_service(repoid=123)
assert isinstance(res, NewMinioStorageService)
assert res.minio_config == minio_config
@@ -168,7 +137,6 @@ def test_get_appropriate_storage_service_new_minio_false(
"minio": minio_config,
}
mocker.patch.object(USE_NEW_MINIO, "check_value", return_value=False)
mocker.patch.object(USE_MINIO, "check_value", return_value=False)
res = get_appropriate_storage_service(repoid=123)
assert isinstance(res, MinioStorageService)
assert res.minio_config == minio_config
@@ -184,8 +152,6 @@ def test_get_appropriate_storage_service_new_minio_cached(
}

mocker.patch.object(USE_NEW_MINIO, "check_value", return_value=False)
mocker.patch.object(USE_MINIO, "check_value", return_value=False)

res = get_appropriate_storage_service(repoid=123)

assert isinstance(res, MinioStorageService)