HDDS-16071. Add S3 Bucket Lifecycle CRUD integration tests to AbstractS3SDKV2Tests - #10986
HDDS-16071. Add S3 Bucket Lifecycle CRUD integration tests to AbstractS3SDKV2Tests#10986NickJavaDev88 wants to merge 5 commits into
Conversation
echonesis
left a comment
There was a problem hiding this comment.
Thanks @NickJavaDev88 for the patch.
| () -> s3Client.putBucketLifecycleConfiguration(b -> b | ||
| .bucket(nonExistentBucket) | ||
| .lifecycleConfiguration(validConfig))); | ||
| assertEquals(404, exception2.statusCode()); |
There was a problem hiding this comment.
Please use HTTP_NOT_FOUND instead of the hard-coded 404, consistent with the existing lifecycle tests.
| assertThrows(S3Exception.class, | ||
| () -> s3Client.getBucketLifecycleConfiguration(b -> b.bucket(bucketName))); |
There was a problem hiding this comment.
Could we assert HTTP_NOT_FOUND and S3ErrorTable.NO_SUCH_LIFECYCLE_CONFIGURATION.getCode(), as mentioned in the PR description? The same applies to the assertions below.
|
Thanks for the review, @echonesis — both addressed in 3b89d36 :
Verified locally: all 4 new lifecycle tests (testS3LifecycleConfigurationCreateSuccessfully, CreationFailed, Delete, Get) pass against a real MiniOzoneCluster in both AbstractS3SDKV1Tests and AbstractS3SDKV2Tests. |
|
Thanks @priyeshkaratha for adding the labels! |
|
I pushed a quick follow-up commit (c849e67) to replace the remaining literal 400 with HttpURLConnection.HTTP_BAD_REQUEST in testS3LifecycleConfigurationCreationFailed. I initially missed this one during my search because I was filtering by 404, but caught it while reviewing the test cases where the bucket exists but lacks configuration. The CI run is currently in progress: https://github.com/NickJavaDev88/ozone/actions/runs/31678412705 |
|
Can you do rebase with latest master? @NickJavaDev88 There are few changes happened which is improving CI run in master. |
|
Thanks for the heads-up, @priyeshkaratha! I've brought the branch up to date with I updated it via |
sreejasahithi
left a comment
There was a problem hiding this comment.
Thanks @NickJavaDev88 for this PR.
Changes LGTM
What changes were proposed in this pull request?
This PR adds basic S3 Bucket Lifecycle CRUD integration tests to
AbstractS3SDKV2Teststo achieve test parity withAbstractS3SDKV1Tests.Key observations ported for AWS SDK v2:
getBucketLifecycleConfigurationthrows anS3Exception(404 /NoSuchLifecycleConfiguration) when no lifecycle configuration exists on a bucket (unlike SDK v1 which returnednull).deleteBucketLifecycleremains idempotent (204 No Content) when deleting a non-existent configuration, matching the fix from HDDS-16005.InvalidRequest(400).What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-16071
How was this patch tested?
checkstyle.sh,rat.sh,author.sh) passed.mvn -pl :ozone-integration-test-s3 test -Dtest=TestS3SDK(194 tests passed).(Note: The single failure in
acceptanceis a known flaky test where SCM gets stuck in safe mode, unrelated to S3 SDK changes).