ayushtkn commented on code in PR #10639:
URL: https://github.com/apache/ozone/pull/10639#discussion_r3503385706
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/RootEndpoint.java:
##########
@@ -153,22 +153,49 @@ private Response listAllBuckets()
long startNanos = Time.monotonicNowNanos();
boolean auditSuccess = true;
try {
+ final String continueToken =
queryParams().get(QueryParams.CONTINUATION_TOKEN);
+ final String maxBucketsParam =
queryParams().get(QueryParams.MAX_BUCKETS);
+ final boolean paginated = maxBucketsParam != null || continueToken !=
null;
+ final int maxBuckets = paginated
+ ? validateMaxBuckets(queryParams().getInt(QueryParams.MAX_BUCKETS,
+ S3Consts.MAX_BUCKETS_LIMIT))
+ : Integer.MAX_VALUE;
Review Comment:
can we not do this logic inside the method itself `validateMaxBuckets`,
actually this isn't validate, it is like `getMaxBuckets`, because it does
```
return Math.min(maxBuckets, S3Consts.MAX_BUCKETS_LIMIT);
```
Maybe we can drop the ternary here and do all this logic inside the method
##########
hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/endpoint/TestRootList.java:
##########
@@ -68,4 +73,95 @@ public void testListBucket() throws Exception {
assertEquals(S3Owner.DEFAULT_S3OWNER_ID, response.getOwner().getId());
}
+ @Test
+ public void testListAllBucketsPaginated() throws Exception {
+ ListBucketResponse response = listWithMaxBuckets(1);
+ assertEquals(0, response.getBucketsNum());
+ assertNull(response.getContinuationToken());
+
+ clientStub.getObjectStore().createS3Bucket("bucket-a");
+ response = listWithMaxBuckets(1);
+ assertEquals(1, response.getBucketsNum());
+ assertEquals("bucket-a", response.getBuckets().get(0).getName());
+ assertNull(response.getContinuationToken());
+
+ clientStub.getObjectStore().createS3Bucket("bucket-b");
+ response = listWithMaxBuckets(1);
+ assertEquals(1, response.getBucketsNum());
+ assertEquals("bucket-a", response.getBuckets().get(0).getName());
+ assertNotNull(response.getContinuationToken());
+
+ rootEndpoint.queryParamsForTest().set(QueryParams.CONTINUATION_TOKEN,
+ response.getContinuationToken());
+ rootEndpoint.queryParamsForTest().setInt(QueryParams.MAX_BUCKETS, 1);
+ response = (ListBucketResponse) rootEndpoint.get().getEntity();
+ assertEquals(1, response.getBucketsNum());
+ assertEquals("bucket-b", response.getBuckets().get(0).getName());
+ assertNull(response.getContinuationToken());
+ }
+
+ @Test
+ public void testListAllBucketsPaginationMultiplePages() throws Exception {
+ String bucketBaseName = "bucket-" + getClass().getName();
+ for (int i = 0; i < 5; i++) {
+ clientStub.getObjectStore().createS3Bucket(bucketBaseName + i);
+ }
+
+ rootEndpoint.queryParamsForTest().setInt(QueryParams.MAX_BUCKETS, 2);
Review Comment:
could the `listWithMaxBuckets` be used here?
##########
hadoop-ozone/integration-test-s3/src/test/java/org/apache/hadoop/ozone/s3/awssdk/v1/AbstractS3SDKV1Tests.java:
##########
@@ -286,6 +288,144 @@ public void testListBuckets() throws IOException {
assertThat(s3AccountOwner.getId()).isEqualTo(S3Owner.DEFAULT_S3OWNER_ID);
}
+ /**
+ * Integration tests for paginated ListBuckets (GET / ListAllMyBuckets).
+ */
+ @Nested
+ class ListBucketsPaginationTests {
+
+ /**
+ * Verifies {@code maxBuckets=1} returns one bucket per page and a
continuation token
+ * when more buckets exist.
+ */
+ @Test
+ public void testListBucketsPaginatedMaxBucketsOne() {
+ final String bucketA = uniqueObjectName("bucket-a");
+ final String bucketB = uniqueObjectName("bucket-b");
+ s3Client.createBucket(bucketA);
+ s3Client.createBucket(bucketB);
+ try {
+ List<String> found = new ArrayList<>();
+ String continuationToken = null;
+
+ do {
+ ListBucketsPaginatedRequest request = new
ListBucketsPaginatedRequest()
+ .withMaxBuckets(1);
+ if (continuationToken != null) {
+ request.withContinuationToken(continuationToken);
+ }
+
+ ListBucketsPaginatedResult page = s3Client.listBuckets(request);
+ assertEquals(1, page.getBuckets().size());
+ found.add(page.getBuckets().get(0).getName());
+ continuationToken = page.getContinuationToken();
+ } while (continuationToken != null
+ && !(found.contains(bucketA) && found.contains(bucketB)));
Review Comment:
the second condition is unnecessary, pagination should naturally terminate
when there are no elements remaining.
We can maybe assert that it returns exactly 2 elements which are `bucketA` &
`bucketB`
##########
hadoop-ozone/integration-test-s3/src/test/java/org/apache/hadoop/ozone/s3/awssdk/v2/AbstractS3SDKV2Tests.java:
##########
@@ -2796,6 +2797,144 @@ private void
verifyBucketOwnershipVerificationAccessDenied(Executable function)
}
}
+ /**
+ * Integration tests for paginated ListBuckets (GET / ListAllMyBuckets).
+ */
+ @Nested
+ class ListBucketsPaginationTests {
+
+ /**
+ * Verifies {@code maxBuckets=1} returns one bucket per page and a
continuation token
+ * when more buckets exist.
+ */
+ @Test
+ public void testListBucketsPaginatedMaxBucketsOne() throws Exception {
Review Comment:
is this a almost a copy from the V1 test, we should ideally refactor to make
sure we are not having the same code both places.
unless that is some established practice for the particular case
##########
hadoop-ozone/integration-test-s3/src/test/java/org/apache/hadoop/ozone/s3/awssdk/v1/AbstractS3SDKV1Tests.java:
##########
@@ -286,6 +288,144 @@ public void testListBuckets() throws IOException {
assertThat(s3AccountOwner.getId()).isEqualTo(S3Owner.DEFAULT_S3OWNER_ID);
}
+ /**
+ * Integration tests for paginated ListBuckets (GET / ListAllMyBuckets).
+ */
+ @Nested
+ class ListBucketsPaginationTests {
+
+ /**
+ * Verifies {@code maxBuckets=1} returns one bucket per page and a
continuation token
+ * when more buckets exist.
+ */
+ @Test
+ public void testListBucketsPaginatedMaxBucketsOne() {
+ final String bucketA = uniqueObjectName("bucket-a");
+ final String bucketB = uniqueObjectName("bucket-b");
+ s3Client.createBucket(bucketA);
+ s3Client.createBucket(bucketB);
+ try {
+ List<String> found = new ArrayList<>();
+ String continuationToken = null;
+
+ do {
+ ListBucketsPaginatedRequest request = new
ListBucketsPaginatedRequest()
+ .withMaxBuckets(1);
+ if (continuationToken != null) {
+ request.withContinuationToken(continuationToken);
+ }
+
+ ListBucketsPaginatedResult page = s3Client.listBuckets(request);
+ assertEquals(1, page.getBuckets().size());
+ found.add(page.getBuckets().get(0).getName());
+ continuationToken = page.getContinuationToken();
+ } while (continuationToken != null
+ && !(found.contains(bucketA) && found.contains(bucketB)));
+
+ assertThat(found).contains(bucketA, bucketB);
Review Comment:
maybe `assertThat(found).containsExactlyInAnyOrder(bucketA,
bucketB);`
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
