Skip to content

HDDS-16709. Remove redundant ListStatusLight RPC from S3 root listings in FSO buckets #11408

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.

Sign up for GitHub

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

Open
rich7420 wants to merge 3 commits into
base: master
Choose a base branch
from
Open

HDDS-16709. Remove redundant ListStatusLight RPC from S3 root listings in FSO buckets #11408

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ee1835f
HDDS-16709. Remove redundant ListStatusLight RPC from S3 root listing…
rich7420 Oct 3, 2026
5cf7609
HDDS-16709. Combine shallow listing initialization branches
rich7420 Oct 7, 2026
9dc94a3
HDDS-16709. Cover shallow root listing with a marker
rich7420 Oct 8, 2026
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
95 changes: 51 additions & 44 deletions hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneBucket.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1884,60 +1884,67 @@ List<OzoneKey> getNextShallowListOfKeys(String prevKey)
if (!addedKeyPrefix()) {
initDelimiterKeyPrefix();

final boolean listRoot = getKeyPrefix().isEmpty() && StringUtils.isEmpty(prevKey);
if (!prepareStack(prevKey)) {
return new ArrayList<>();
}

// 1. Get first element as startKey.
List<OzoneKey> firstKeyResult = new ArrayList<>();
if (stack.isEmpty()) {
// Case: startKey is empty
getChildrenKeys(getKeyPrefix(), prevKey, firstKeyResult);
if (listRoot) {
startKey = "";
findFirstStartKey = true;
setAddedKeyPrefix(true);
} else {
// Case: startKey is non-empty
while (!stack.isEmpty()) {
Pair<String, String> keyPrefixPath = stack.pop();
getChildrenKeys(keyPrefixPath.getLeft(), keyPrefixPath.getRight(),
firstKeyResult);
if (!firstKeyResult.isEmpty()) {
break;
// 1. Get first element as startKey.
List<OzoneKey> firstKeyResult = new ArrayList<>();
if (stack.isEmpty()) {
// Case: startKey is empty
getChildrenKeys(getKeyPrefix(), prevKey, firstKeyResult);
} else {
// Case: startKey is non-empty
while (!stack.isEmpty()) {
Pair<String, String> keyPrefixPath = stack.pop();
getChildrenKeys(keyPrefixPath.getLeft(), keyPrefixPath.getRight(),
firstKeyResult);
if (!firstKeyResult.isEmpty()) {
break;
}
}
}
}
if (!firstKeyResult.isEmpty()) {
startKey = firstKeyResult.get(0).getName();
findFirstStartKey = true;
}
if (!firstKeyResult.isEmpty()) {
startKey = firstKeyResult.get(0).getName();
findFirstStartKey = true;
}

// A specific case where findFirstStartKey is false does not mean that
// the final result is empty because also need to determine whether
// keyPrefix is an existing key. Consider the following structure:
// te/
// test1/
// test1/file1
// test1/file2
// test2/
// when keyPrefix='te' and prevKey='test1/file2', findFirstStartKey
// will be false because 'test1/file2' is the last key in dir
// 'test1/'. In the correct result for this case, 'test2/' is expected
// in the results and "test1/" should be excluded.
//
if (!findFirstStartKey) {
if (StringUtils.isBlank(prevKey) || !keyPrefixExist()
|| !StringUtils.startsWith(prevKey, getKeyPrefix())) {
return new ArrayList<>();
// A specific case where findFirstStartKey is false does not mean that
// the final result is empty because also need to determine whether
// keyPrefix is an existing key. Consider the following structure:
// te/
// test1/
// test1/file1
// test1/file2
// test2/
// when keyPrefix='te' and prevKey='test1/file2', findFirstStartKey
// will be false because 'test1/file2' is the last key in dir
// 'test1/'. In the correct result for this case, 'test2/' is expected
// in the results and "test1/" should be excluded.
//
if (!findFirstStartKey) {
if (StringUtils.isBlank(prevKey) || !keyPrefixExist()
|| !StringUtils.startsWith(prevKey, getKeyPrefix())) {
return new ArrayList<>();
}
}
// A special case where keyPrefix element should present in the
// resultList. See the annotation of #addKeyPrefixInfoToResultList
if (getKeyPrefix().equals(startKey) && findFirstStartKey
&& !firstKeyResult.get(0).isFile()) {
resultList.add(firstKeyResult.get(0));
}
// Note that the startKey needs to be an immediate child of the
// keyPrefix or black before calling listStatus.
startKey = adjustStartKey(startKey);
startKey = startKey == null ? "" : startKey;
}
// A special case where keyPrefix element should present in the
// resultList. See the annotation of #addKeyPrefixInfoToResultList
if (getKeyPrefix().equals(startKey) && findFirstStartKey
&& !firstKeyResult.get(0).isFile()) {
resultList.add(firstKeyResult.get(0));
}
// Note that the startKey needs to be an immediate child of the
// keyPrefix or black before calling listStatus.
startKey = adjustStartKey(startKey);
startKey = startKey == null ? "" : startKey;
}

// 2. Get immediate children by listStatus method.
Expand Down
164 changes: 164 additions & 0 deletions hadoop-ozone/client/src/test/java/org/apache/hadoop/ozone/client/TestOzoneBucket.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,17 +17,44 @@

package org.apache.hadoop.ozone.client;

import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_CLIENT_LIST_CACHE_SIZE;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertSame;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.CALLS_REAL_METHODS;
import static org.mockito.Mockito.doReturn;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.verifyNoMoreInteractions;
import static org.mockito.Mockito.when;

import java.io.IOException;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Iterator;
import java.util.List;
import java.util.stream.IntStream;
import java.util.stream.Stream;
import org.apache.hadoop.hdds.client.RatisReplicationConfig;
import org.apache.hadoop.hdds.conf.OzoneConfiguration;
import org.apache.hadoop.hdds.protocol.proto.HddsProtos.ReplicationFactor;
import org.apache.hadoop.ozone.client.protocol.ClientProtocol;
import org.apache.hadoop.ozone.client.protocol.ListStatusLightOptions;
import org.apache.hadoop.ozone.om.helpers.BasicOmKeyInfo;
import org.apache.hadoop.ozone.om.helpers.BucketLayout;
import org.apache.hadoop.ozone.om.helpers.OzoneFSUtils;
import org.apache.hadoop.ozone.om.helpers.OzoneFileStatus;
import org.apache.hadoop.ozone.om.helpers.OzoneFileStatusLight;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.Arguments;
import org.junit.jupiter.params.provider.MethodSource;
import org.junit.jupiter.params.provider.NullAndEmptySource;
import org.junit.jupiter.params.provider.ValueSource;
import org.mockito.ArgumentCaptor;

/**
* Unit tests for {@link OzoneBucket}.
Expand Down Expand Up @@ -69,4 +96,141 @@ public void clientProtocol3argDefaultDelegates() throws IOException {
assertSame(status, proxy.getOzoneFileStatus("vol", "bucket", "key"));
verify(proxy).getOzoneFileStatus("vol", "bucket", "key", false);
}

@ParameterizedTest
@NullAndEmptySource
void shallowRootListingStartsAtRoot(String prevKey) throws IOException {
ClientProtocol proxy = mock(ClientProtocol.class);
when(proxy.listStatusLight(any())).thenAnswer(invocation -> new ArrayList<>(Arrays.asList(
keyStatus("a-file", false), keyStatus("b-dir", true), keyStatus("c-file", false))));

Iterator<? extends OzoneKey> keys = fsoBucket(proxy).listKeys("", prevKey, true);

ArgumentCaptor<ListStatusLightOptions> options = ArgumentCaptor.forClass(ListStatusLightOptions.class);
verify(proxy).listStatusLight(options.capture());
assertEquals("vol", options.getValue().getVolumeName());
assertEquals("bucket", options.getValue().getBucketName());
assertEquals("", options.getValue().getKeyName());
assertEquals("", options.getValue().getStartKey());
assertEquals("", options.getValue().getListPrefix());
assertEquals(3, options.getValue().getNumEntries());
assertFalse(options.getValue().isRecursive());
assertFalse(options.getValue().isAllowPartialPrefixes());
verifyNoMoreInteractions(proxy);

OzoneKey file = keys.next();
assertEquals("a-file", file.getName());
assertEquals("owner", file.getOwner());
assertEquals(10, file.getDataSize());
assertEquals(RatisReplicationConfig.getInstance(ReplicationFactor.ONE), file.getReplicationConfig());
OzoneKey directory = keys.next();
assertEquals("b-dir/", directory.getName());
assertFalse(directory.isFile());
}

@ParameterizedTest
@MethodSource("rootListings")
void shallowRootListingPreservesPages(String shape, int size, String prevKey) throws IOException {
ClientProtocol proxy = mock(ClientProtocol.class);
List<OzoneFileStatusLight> statuses = new ArrayList<>();
List<String> expected = new ArrayList<>();
for (int i = 0; i < size; i++) {
String name = "key-" + i;
boolean directory = shape.equals("directories") || (shape.equals("mixed") && i % 2 == 0);
statuses.add(keyStatus(name, directory));
expected.add(name + (directory ? "/" : ""));
}
when(proxy.listStatusLight(any())).thenAnswer(invocation -> {
ListStatusLightOptions options = invocation.getArgument(0);
String startKey = OzoneFSUtils.removeTrailingSlashIfNeeded(options.getStartKey());
int start = 0;
while (start < statuses.size() && statuses.get(start).getTrimmedName().compareTo(startKey) < 0) {
start++;
}
int end = Math.min(statuses.size(), start + (int) options.getNumEntries());
return new ArrayList<>(statuses.subList(start, end));
});

Iterator<? extends OzoneKey> keys = fsoBucket(proxy).listKeys("", prevKey, true);
verify(proxy).listStatusLight(any());
verifyNoMoreInteractions(proxy);
List<String> actual = new ArrayList<>();
keys.forEachRemaining(key -> actual.add(key.getName()));
assertEquals(expected, actual);
}

private static Stream<Arguments> rootListings() {
return Stream.of("files", "directories", "mixed").flatMap(shape ->
IntStream.of(0, 1, 3, 4, 6, 7).boxed().flatMap(size ->
Stream.of(Arguments.of(shape, size, null), Arguments.of(shape, size, ""))));
}

@Test
void shallowRootListingWithMarkerKeepsSeed() throws IOException {
ClientProtocol proxy = mock(ClientProtocol.class);
when(proxy.listStatusLight(any())).thenAnswer(invocation -> {
ListStatusLightOptions options = invocation.getArgument(0);
List<OzoneFileStatusLight> statuses = new ArrayList<>(Arrays.asList(
keyStatus("a-file", false), keyStatus("b-file", false)));
statuses.removeIf(status -> status.getTrimmedName().compareTo(options.getStartKey()) < 0);
return statuses;
});

Iterator<? extends OzoneKey> keys = fsoBucket(proxy).listKeys("", "a-file", true);

ArgumentCaptor<ListStatusLightOptions> options = ArgumentCaptor.forClass(ListStatusLightOptions.class);
verify(proxy, times(2)).listStatusLight(options.capture());
for (ListStatusLightOptions call : options.getAllValues()) {
assertEquals("vol", call.getVolumeName());
assertEquals("bucket", call.getBucketName());
assertEquals("", call.getKeyName());
assertEquals("", call.getListPrefix());
assertEquals(3, call.getNumEntries());
assertFalse(call.isRecursive());
}
assertEquals("a-file", options.getAllValues().get(0).getStartKey());
assertTrue(options.getAllValues().get(0).isAllowPartialPrefixes());
assertEquals("b-file", options.getAllValues().get(1).getStartKey());
assertFalse(options.getAllValues().get(1).isAllowPartialPrefixes());
verifyNoMoreInteractions(proxy);
assertEquals("b-file", keys.next().getName());
}

@ParameterizedTest
@ValueSource(strings = {"dir/", "/"})
void shallowListingWithPrefixKeepsSeed(String prefix) throws IOException {
ClientProtocol proxy = mock(ClientProtocol.class);
when(proxy.listStatusLight(any())).thenAnswer(invocation ->
new ArrayList<>(Arrays.asList(keyStatus(prefix + "file", false))));

fsoBucket(proxy).listKeys(prefix, null, true);

ArgumentCaptor<ListStatusLightOptions> options = ArgumentCaptor.forClass(ListStatusLightOptions.class);
verify(proxy, times(2)).listStatusLight(options.capture());
assertTrue(options.getAllValues().get(0).isAllowPartialPrefixes());
assertFalse(options.getAllValues().get(1).isAllowPartialPrefixes());
Comment on lines +199 to +211

@echonesis echonesis Oct 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Could we also add a case with an empty prefix and a non-empty prevKey? This would verify that marker-based listings still make both ListStatusLight calls and exclude the marker from the results.

}

private static OzoneBucket fsoBucket(ClientProtocol proxy) {
OzoneConfiguration conf = new OzoneConfiguration();
conf.setInt(OZONE_CLIENT_LIST_CACHE_SIZE, 3);
return OzoneBucket.newBuilder(conf, proxy)
.setVolumeName("vol")
.setName("bucket")
.setBucketLayout(BucketLayout.FILE_SYSTEM_OPTIMIZED)
.build();
}

private static OzoneFileStatusLight keyStatus(String name, boolean directory) {
BasicOmKeyInfo keyInfo = new BasicOmKeyInfo.Builder()
.setVolumeName("vol")
.setBucketName("bucket")
.setKeyName(name)
.setDataSize(10)
.setOwnerName("owner")
.setReplicationConfig(RatisReplicationConfig.getInstance(ReplicationFactor.ONE))
.setIsFile(!directory)
.build();
return new OzoneFileStatusLight(keyInfo, 0, directory);
}
}
25 changes: 25 additions & 0 deletions ...-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestListKeysWithFSO.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@
import java.io.InputStream;
import java.nio.charset.StandardCharsets;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.Iterator;
import java.util.LinkedList;
import java.util.List;
Expand Down Expand Up @@ -505,6 +507,29 @@ public void testShallowListKeys() throws Exception {
checkKeyShallowList(keyPrefix, startKey, expectedKeys, fsoOzoneBucket);
}

@Test
void testShallowListKeysAtBucketRoot() throws Exception {
OzoneBucket bucket = DataTestUtil.createVolumeAndBucket(client, BucketLayout.FILE_SYSTEM_OPTIMIZED);
checkKeyShallowList("", null, Collections.emptyList(), bucket);
bucket.createDirectory("a");
checkKeyShallowList("", null, Collections.singletonList("a/"), bucket);

List<String> files = Arrays.asList("b0", "b1", "b2", "b3", "b4");
createAndAssertKeys(bucket, files);
List<String> expected = new ArrayList<>();
expected.add("a/");
expected.addAll(files);
checkKeyShallowList("", null, expected, bucket);
checkKeyShallowList("", "", expected, bucket);

createAndAssertKeys(bucket, Collections.singletonList("b5"));
expected.add("b5");
checkKeyShallowList("", null, expected, bucket);
checkKeyShallowList("", "", expected, bucket);
checkKeyShallowList("", "b2", Arrays.asList("b3", "b4", "b5"), bucket);
checkKeyShallowList("b", null, Arrays.asList("b0", "b1", "b2", "b3", "b4", "b5"), bucket);
}

@Test
void testIsFileFalseForDir() throws Exception {
byte[] data = "key-data".getBytes(StandardCharsets.UTF_8);
Expand Down
Loading