Yicong-Huang commented on code in PR #8262:
URL: https://github.com/apache/texera/pull/8262#discussion_r4058823815
##########
amber/src/test/python/pytexera/storage/test_large_binary_manager.py:
##########
@@ -128,6 +129,33 @@ def
test_ensure_bucket_exists_creates_bucket_when_missing(self):
mock_client.head_bucket.assert_called_once_with(Bucket="test-bucket")
mock_client.create_bucket.assert_called_once_with(Bucket="test-bucket")
+ def test_ensure_bucket_exists_creates_bucket_on_head_404(self):
Review Comment:
**Must fix:**
Retitle to `feat(pyamber): create missing large binary buckets`. `pytexera`
is a package inside the pyamber module, not a module scope. AGENTS.md:144-146
says to use the module name, and the labeler put `pyamber` on this PR. As a
scope, `pytexera` appears in zero of the last 1500 merged commits, against
dozens for `pyamber`. Worth re-confirming `feat` over `fix` while you are
editing: AGENTS.md:160 does support `feat` for implementing something that was
never actually there, but the linked issue is a defect report, and the type
decides release-branch eligibility.
##########
amber/src/main/python/pytexera/storage/large_binary_manager.py:
##########
@@ -84,7 +85,15 @@ def _ensure_bucket_exists(self, bucket: str):
s3 = self._get_s3_client()
try:
s3.head_bucket(Bucket=bucket)
- except s3.exceptions.NoSuchBucket:
+ except (s3.exceptions.NoSuchBucket, ClientError) as error:
+ if isinstance(error, ClientError) and error.response["Error"][
+ "Code"
+ ] not in (
+ "404",
+ "NoSuchBucket",
+ "NotFound",
+ ):
+ raise
logger.debug(f"Bucket {bucket} not found, creating it")
s3.create_bucket(Bucket=bucket)
Review Comment:
**Advisory:**
This path does not tolerate the bucket already being there by the time
CreateBucket runs. Two writers that both get the missing-bucket answer both
call it, and the loser gets `BucketAlreadyOwnedByYou`, which `upload_worker`
stores and re-raises as a failed upload rather than a retry. It is advisory
because I could not name a path a reader could walk: `FileService.scala:81`
creates this bucket at boot, so HeadBucket succeeds and this branch is never
entered. Is the Python side meant to be the real creator, or a fallback behind
that one? Either way, treating the already-exists codes as success costs one
condition.
##########
amber/src/main/python/pytexera/storage/large_binary_manager.py:
##########
@@ -84,7 +85,15 @@ def _ensure_bucket_exists(self, bucket: str):
s3 = self._get_s3_client()
try:
s3.head_bucket(Bucket=bucket)
- except s3.exceptions.NoSuchBucket:
+ except (s3.exceptions.NoSuchBucket, ClientError) as error:
+ if isinstance(error, ClientError) and error.response["Error"][
+ "Code"
+ ] not in (
+ "404",
+ "NoSuchBucket",
+ "NotFound",
+ ):
+ raise
Review Comment:
**Advisory:**
Under real botocore the modeled `NoSuchBucket` is itself a `ClientError`, so
the first arm of the tuple is unreachable in production. It is also what forces
the `isinstance` test and splits the code lookup over three lines. The tests
cannot show this either way, since they fabricate the class as a bare
`Exception` (test_large_binary_manager.py:134). Is there a client where the
modeled exception arrives separately? If not, one `except ClientError` reads
better, and the reason it is enough is what a future reader will stop on.
```suggestion
except ClientError as error:
# HeadBucket answers a miss with a bodyless 404, so the modeled
# NoSuchBucket (itself a ClientError) never reaches this handler.
missing = ("404", "NoSuchBucket", "NotFound")
if error.response["Error"]["Code"] not in missing:
raise
```
##########
amber/src/test/python/pytexera/storage/test_large_binary_manager.py:
##########
@@ -128,6 +129,33 @@ def
test_ensure_bucket_exists_creates_bucket_when_missing(self):
mock_client.head_bucket.assert_called_once_with(Bucket="test-bucket")
mock_client.create_bucket.assert_called_once_with(Bucket="test-bucket")
+ def test_ensure_bucket_exists_creates_bucket_on_head_404(self):
+ mock_client = MagicMock()
+ mock_client.exceptions.NoSuchBucket = type("NoSuchBucket",
(Exception,), {})
+ mock_client.head_bucket.side_effect = ClientError(
+ {"Error": {"Code": "404", "Message": "Not Found"}}, "HeadBucket"
+ )
+ large_binary_manager._s3_client = mock_client
Review Comment:
**Polish:**
These two tests assign the process-wide singleton's `_s3_client` and never
put it back, where every other test in the class resets it to `None` and
patches `boto3.client` first. It is harmless today because the siblings patch
`_get_s3_client` wholesale, but it leaves a `MagicMock` cached for whatever
runs next.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
