From 7042c0b54b20f6aae86ceb93540df287b6464687 Mon Sep 17 00:00:00 2001 From: KUAN-HAO HUANG <101171023+rich7420@users.noreply.github.com> Date: Fri, 2 Oct 2026 17:16:33 +0800 Subject: [PATCH 1/2] HDDS-16673. Avoid redundant ListBuckets RPCs when S3 bucket listing reaches the end --- .../ozone/s3/endpoint/RootEndpoint.java | 8 +- .../ozone/s3/endpoint/TestRootList.java | 109 +++++++++++++++++- 2 files changed, 112 insertions(+), 5 deletions(-) diff --git a/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/RootEndpoint.java b/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/RootEndpoint.java index f5d24fa8de26..1af40b7094d5 100644 --- a/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/RootEndpoint.java +++ b/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/RootEndpoint.java @@ -114,7 +114,7 @@ private Response listDirectoryBuckets() String accountId = S3Owner.DEFAULT_S3OWNER_ID; int count = 0; String lastBucketName = null; - while (bucketIterator.hasNext() && count < maxDirectoryBuckets) { + while (count < maxDirectoryBuckets && bucketIterator.hasNext()) { OzoneBucket bucket = bucketIterator.next(); if (!bucket.getBucketLayout().isFileSystemOptimized()) { continue; @@ -130,7 +130,7 @@ private Response listDirectoryBuckets() count++; } - if (lastBucketName != null && bucketIterator.hasNext()) { + if (count == maxDirectoryBuckets && lastBucketName != null && bucketIterator.hasNext()) { response.setContinuationToken( new ContinueToken(lastBucketName, null).encodeToString()); } @@ -182,7 +182,7 @@ private Response listAllBuckets() int count = 0; String lastBucketName = null; - while (bucketIterator.hasNext() && count < maxBuckets) { + while (count < maxBuckets && bucketIterator.hasNext()) { OzoneBucket next = bucketIterator.next(); BucketMetadata bucketMetadata = new BucketMetadata(); bucketMetadata.setName(next.getName()); @@ -192,7 +192,7 @@ private Response listAllBuckets() count++; } - if (paginated && lastBucketName != null && bucketIterator.hasNext()) { + if (paginated && count == maxBuckets && lastBucketName != null && bucketIterator.hasNext()) { response.setContinuationToken( new ContinueToken(lastBucketName, null).encodeToString()); } diff --git a/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/endpoint/TestRootList.java b/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/endpoint/TestRootList.java index 4622564e743b..2855da8bb72e 100644 --- a/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/endpoint/TestRootList.java +++ b/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/endpoint/TestRootList.java @@ -17,18 +17,41 @@ package org.apache.hadoop.ozone.s3.endpoint; +import static org.assertj.core.api.Assertions.assertThat; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertThrows; - +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.ArgumentMatchers.isNull; +import static org.mockito.ArgumentMatchers.nullable; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; +import java.util.stream.Collectors; +import javax.ws.rs.core.Response; +import org.apache.hadoop.hdds.conf.OzoneConfiguration; import org.apache.hadoop.ozone.OzoneConfigKeys; +import org.apache.hadoop.ozone.client.ObjectStore; +import org.apache.hadoop.ozone.client.OzoneBucket; import org.apache.hadoop.ozone.client.OzoneClient; import org.apache.hadoop.ozone.client.OzoneClientStub; +import org.apache.hadoop.ozone.client.OzoneVolume; +import org.apache.hadoop.ozone.client.protocol.ClientProtocol; +import org.apache.hadoop.ozone.om.helpers.BucketLayout; import org.apache.hadoop.ozone.s3.exception.OS3Exception; +import org.apache.hadoop.ozone.s3.signature.SignatureInfo; +import org.apache.hadoop.ozone.s3.util.ContinueToken; import org.apache.hadoop.ozone.s3.util.S3Consts.QueryParams; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.CsvSource; /** * This class test HeadBucket functionality. @@ -156,6 +179,90 @@ public void testListAllBucketsUnpaginatedReturnsAll() throws Exception { assertNull(response.getContinuationToken()); } + @ParameterizedTest + @CsvSource({ + "false, 0, 2, -1, 0, false, 1", + "false, 1, 2, -1, 1, false, 2", + "false, 2, 2, -1, 2, false, 2", + "false, 3, 2, -1, 2, true, 2", + "false, 5, 2, -1, 2, true, 2", + "false, 3, 5, -1, 3, false, 3", + "false, 3, 2, 1, 1, false, 2", + "true, 0, 2, -1, 0, false, 1", + "true, 1, 2, -1, 1, false, 2", + "true, 2, 2, -1, 1, false, 2", + "true, 3, 2, -1, 2, false, 3", + "true, 4, 2, -1, 2, true, 2", + "true, 3, 5, -1, 2, false, 3", + "true, 3, 0, -1, 0, false, 1", + "true, 5, 2, 2, 1, false, 2" + }) + void testListBucketsRpcCount(boolean directory, int total, int limit, int previous, + int expectedCount, boolean hasToken, int expectedCalls) throws Exception { + OzoneConfiguration conf = new OzoneConfiguration(); + conf.setInt(OzoneConfigKeys.OZONE_CLIENT_LIST_CACHE_SIZE, 2); + ClientProtocol proxy = mock(ClientProtocol.class); + List buckets = new ArrayList<>(); + for (int i = 0; i < total; i++) { + buckets.add(OzoneBucket.newBuilder(conf, proxy).setVolumeName(DEFAULT_VOLUME).setName("bucket-" + i) + .setBucketLayout(i % 2 == 0 ? BucketLayout.FILE_SYSTEM_OPTIMIZED : BucketLayout.OBJECT_STORE).build()); + } + when(proxy.listBuckets(eq(DEFAULT_VOLUME), isNull(), nullable(String.class), eq(2), eq(false))) + .thenAnswer(invocation -> { + String marker = invocation.getArgument(2); + List remaining = buckets.stream() + .filter(bucket -> marker == null || bucket.getName().compareTo(marker) > 0) + .limit(2).collect(Collectors.toList()); + return remaining; + }); + OzoneVolume volume = OzoneVolume.newBuilder(conf, proxy).setName(DEFAULT_VOLUME).setOwner("root") + .setAcls(Collections.emptyList()).build(); + ObjectStore store = mock(ObjectStore.class); + when(store.getS3Volume()).thenReturn(volume); + when(store.getClientProxy()).thenReturn(proxy); + OzoneClient client = mock(OzoneClient.class); + when(client.getObjectStore()).thenReturn(store); + when(client.getProxy()).thenReturn(proxy); + RootEndpoint endpoint = EndpointBuilder.newRootEndpointBuilder().setClient(client) + .setSignatureInfo(new SignatureInfo.Builder(SignatureInfo.Version.V4) + .setCredentialScope("20260101/us-west-2/" + (directory ? "s3express" : "s3") + "/aws4_request").build()) + .build(); + endpoint.queryParamsForTest().setInt( + directory ? QueryParams.MAX_DIRECTORY_BUCKETS : QueryParams.MAX_BUCKETS, limit); + if (previous >= 0) { + endpoint.queryParamsForTest().set(QueryParams.CONTINUATION_TOKEN, + new ContinueToken("bucket-" + previous, null).encodeToString()); + } + try (Response httpResponse = endpoint.get()) { + Object entity = httpResponse.getEntity(); + List names; + String token; + if (directory) { + ListDirectoryBucketsResponse response = (ListDirectoryBucketsResponse) entity; + names = response.getBuckets().stream().map(bucket -> bucket.getName()).collect(Collectors.toList()); + token = response.getContinuationToken(); + } else { + ListBucketResponse response = (ListBucketResponse) entity; + names = response.getBuckets().stream().map(bucket -> bucket.getName()).collect(Collectors.toList()); + token = response.getContinuationToken(); + assertThat(response.getOwner().getDisplayName()).isEqualTo("root"); + } + assertThat(names).hasSize(expectedCount); + assertThat(names).containsExactlyElementsOf(buckets.stream() + .filter(bucket -> previous < 0 || bucket.getName().compareTo("bucket-" + previous) > 0) + .filter(bucket -> !directory || bucket.getBucketLayout().isFileSystemOptimized()).limit(limit) + .map(OzoneBucket::getName).collect(Collectors.toList())); + if (hasToken) { + assertThat(token).isNotNull(); + assertThat(ContinueToken.decodeFromString(token).getLastKey()).isEqualTo(names.get(names.size() - 1)); + } else { + assertThat(token).isNull(); + } + } + verify(proxy, times(expectedCalls)) + .listBuckets(eq(DEFAULT_VOLUME), isNull(), nullable(String.class), eq(2), eq(false)); + } + private ListBucketResponse listWithMaxBuckets(int maxBuckets) throws Exception { rootEndpoint.queryParamsForTest().unset(QueryParams.CONTINUATION_TOKEN); rootEndpoint.queryParamsForTest().setInt(QueryParams.MAX_BUCKETS, maxBuckets); From 7143ec2b3d2449c619026a269918cafe48f279ac Mon Sep 17 00:00:00 2001 From: KUAN-HAO HUANG <101171023+rich7420@users.noreply.github.com> Date: Fri, 2 Oct 2026 17:24:29 +0800 Subject: [PATCH 2/2] HDDS-16673. Clarify bucket listing regression coverage --- .../ozone/s3/endpoint/TestRootList.java | 56 ++++++++++--------- 1 file changed, 29 insertions(+), 27 deletions(-) diff --git a/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/endpoint/TestRootList.java b/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/endpoint/TestRootList.java index 2855da8bb72e..c90967afea78 100644 --- a/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/endpoint/TestRootList.java +++ b/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/endpoint/TestRootList.java @@ -35,6 +35,7 @@ import java.util.List; import java.util.stream.Collectors; import javax.ws.rs.core.Response; +import org.apache.commons.lang3.StringUtils; import org.apache.hadoop.hdds.conf.OzoneConfiguration; import org.apache.hadoop.ozone.OzoneConfigKeys; import org.apache.hadoop.ozone.client.ObjectStore; @@ -179,26 +180,30 @@ public void testListAllBucketsUnpaginatedReturnsAll() throws Exception { assertNull(response.getContinuationToken()); } - @ParameterizedTest + @ParameterizedTest(name = "directory={0}, buckets={1}, limit={2}, previous={3}") @CsvSource({ - "false, 0, 2, -1, 0, false, 1", - "false, 1, 2, -1, 1, false, 2", - "false, 2, 2, -1, 2, false, 2", - "false, 3, 2, -1, 2, true, 2", - "false, 5, 2, -1, 2, true, 2", - "false, 3, 5, -1, 3, false, 3", - "false, 3, 2, 1, 1, false, 2", - "true, 0, 2, -1, 0, false, 1", - "true, 1, 2, -1, 1, false, 2", - "true, 2, 2, -1, 1, false, 2", - "true, 3, 2, -1, 2, false, 3", - "true, 4, 2, -1, 2, true, 2", - "true, 3, 5, -1, 2, false, 3", - "true, 3, 0, -1, 0, false, 1", - "true, 5, 2, 2, 1, false, 2" + // directory, total buckets, response limit, previous index, expected names, continuation token, RPC calls + "false, 0, 2, -1, '', false, 1", + "false, 1, 2, -1, bucket-0, false, 2", + "false, 2, 2, -1, bucket-0;bucket-1, false, 2", + "false, 3, 2, -1, bucket-0;bucket-1, true, 2", + "false, 2, 1, -1, bucket-0, true, 1", + "false, 3, 5, -1, bucket-0;bucket-1;bucket-2, false, 3", + "false, 3, 2, 1, bucket-2, false, 2", + "false, 3, , -1, bucket-0;bucket-1;bucket-2, false, 3", + "true, 0, 2, -1, '', false, 1", + "true, 1, 2, -1, bucket-0, false, 2", + "true, 2, 2, -1, bucket-0, false, 2", + "true, 3, 2, -1, bucket-0;bucket-2, false, 3", + "true, 4, 2, -1, bucket-0;bucket-2, true, 2", + "true, 2, 1, -1, bucket-0, true, 1", + "true, 3, 5, -1, bucket-0;bucket-2, false, 3", + "true, 3, 0, -1, '', false, 1", + "true, 5, 2, 2, bucket-4, false, 2", + "true, 2, 2, 0, '', false, 2" }) - void testListBucketsRpcCount(boolean directory, int total, int limit, int previous, - int expectedCount, boolean hasToken, int expectedCalls) throws Exception { + void testListBucketsRpcCount(boolean directory, int total, Integer limit, int previous, + String expectedNames, boolean hasToken, int expectedCalls) throws Exception { OzoneConfiguration conf = new OzoneConfiguration(); conf.setInt(OzoneConfigKeys.OZONE_CLIENT_LIST_CACHE_SIZE, 2); ClientProtocol proxy = mock(ClientProtocol.class); @@ -210,10 +215,9 @@ void testListBucketsRpcCount(boolean directory, int total, int limit, int previo when(proxy.listBuckets(eq(DEFAULT_VOLUME), isNull(), nullable(String.class), eq(2), eq(false))) .thenAnswer(invocation -> { String marker = invocation.getArgument(2); - List remaining = buckets.stream() + return buckets.stream() .filter(bucket -> marker == null || bucket.getName().compareTo(marker) > 0) .limit(2).collect(Collectors.toList()); - return remaining; }); OzoneVolume volume = OzoneVolume.newBuilder(conf, proxy).setName(DEFAULT_VOLUME).setOwner("root") .setAcls(Collections.emptyList()).build(); @@ -227,8 +231,10 @@ void testListBucketsRpcCount(boolean directory, int total, int limit, int previo .setSignatureInfo(new SignatureInfo.Builder(SignatureInfo.Version.V4) .setCredentialScope("20260101/us-west-2/" + (directory ? "s3express" : "s3") + "/aws4_request").build()) .build(); - endpoint.queryParamsForTest().setInt( - directory ? QueryParams.MAX_DIRECTORY_BUCKETS : QueryParams.MAX_BUCKETS, limit); + if (limit != null) { + endpoint.queryParamsForTest().setInt( + directory ? QueryParams.MAX_DIRECTORY_BUCKETS : QueryParams.MAX_BUCKETS, limit); + } if (previous >= 0) { endpoint.queryParamsForTest().set(QueryParams.CONTINUATION_TOKEN, new ContinueToken("bucket-" + previous, null).encodeToString()); @@ -247,11 +253,7 @@ void testListBucketsRpcCount(boolean directory, int total, int limit, int previo token = response.getContinuationToken(); assertThat(response.getOwner().getDisplayName()).isEqualTo("root"); } - assertThat(names).hasSize(expectedCount); - assertThat(names).containsExactlyElementsOf(buckets.stream() - .filter(bucket -> previous < 0 || bucket.getName().compareTo("bucket-" + previous) > 0) - .filter(bucket -> !directory || bucket.getBucketLayout().isFileSystemOptimized()).limit(limit) - .map(OzoneBucket::getName).collect(Collectors.toList())); + assertThat(names).containsExactly(StringUtils.split(expectedNames, ';')); if (hasToken) { assertThat(token).isNotNull(); assertThat(ContinueToken.decodeFromString(token).getLastKey()).isEqualTo(names.get(names.size() - 1));