Skip to content

HDDS-10562. Fix endless loop in ozone fs -ls #6416

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

Merged
adoroszlai merged 6 commits into from
Mar 24, 2024
Merged

HDDS-10562. Fix endless loop in ozone fs -ls #6416

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2d1d3fa
HDDS-10562. Fix endless loop in ozone sh -ls
symious Mar 21, 2024
f64435d
HDDS-10562. Reduce bucket create count
symious Mar 22, 2024
0e16075
HDDS-10562. Add timeout
symious Mar 22, 2024
4685e86
Update hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/…
symious Mar 22, 2024
6d49531
Update hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/…
symious Mar 22, 2024
71bd923
HDDS-10562. Fix compile
symious Mar 22, 2024
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
30 changes: 30 additions & 0 deletions ...-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/shell/TestOzoneShellHA.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import java.io.IOException;
import java.io.PrintStream;
import java.io.UnsupportedEncodingException;
import java.net.URI;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.HashSet;
Expand Down Expand Up @@ -76,6 +77,7 @@
import static org.apache.hadoop.fs.CommonConfigurationKeysPublic.FS_TRASH_INTERVAL_KEY;
import static org.apache.hadoop.fs.FileSystem.FS_DEFAULT_NAME_KEY;
import static org.apache.hadoop.fs.FileSystem.TRASH_PREFIX;
import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_FS_LISTING_PAGE_SIZE;
import static org.apache.hadoop.ozone.OzoneConsts.OZONE_OFS_URI_SCHEME;

import static org.apache.hadoop.ozone.OzoneConsts.OZONE_URI_DELIMITER;
Expand Down Expand Up @@ -753,6 +755,34 @@ public void testLinkBucketOrphan() throws Exception {
}
}

@Test
@Timeout(10)
public void testListBucket() throws Exception {
final String hostPrefix = OZONE_OFS_URI_SCHEME + "://" + omServiceId;
OzoneConfiguration clientConf =
getClientConfForOFS(hostPrefix, cluster.getConf());
int pageSize = 20;
clientConf.setInt(OZONE_FS_LISTING_PAGE_SIZE, pageSize);
URI uri = FileSystem.getDefaultUri(clientConf);
clientConf.setBoolean(String.format("fs.%s.impl.disable.cache", uri.getScheme()), true);
OzoneFsShell shell = new OzoneFsShell(clientConf);

String volName = "testlistbucket";
int numBuckets = pageSize;
Copy link
Contributor

@adoroszlai adoroszlai Mar 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Should it be pageSize + 1?

Copy link
Contributor Author

@symious symious Mar 22, 2024

Choose a reason for hiding this comment

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

pageSize will also trigger the bug.

Copy link
Contributor Author

@symious symious Mar 22, 2024

Choose a reason for hiding this comment

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

@adoroszlai I wonder if there is a timeout wrapper on some code block in unit test, so that it can fail fast?

Copy link
Contributor

@adoroszlai adoroszlai Mar 22, 2024

Choose a reason for hiding this comment

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

Add @Timeout(seconds) on the test method, e.g.

ozone/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/freon/TestRandomKeyGenerator.java

Lines 72 to 73 in 97038ef

@Test
@Timeout(5)

Copy link
Contributor Author

@symious symious Mar 22, 2024

Choose a reason for hiding this comment

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

@adoroszlai Thanks for the advice. Updated, PTAL.

I was also wondering if there is something like

timout.wrapper(ToolRunner.run(shell, new String[]{"-ls", "/" + volName}));


try {
generateBuckets("/" + volName, numBuckets);
out.reset();
int res = ToolRunner.run(shell, new String[]{"-ls", "/" + volName});
assertEquals(0, res);
String r = out.toString(DEFAULT_ENCODING);
assertThat(r).matches("(?s)^Found " + numBuckets + " items.*");

} finally {
shell.close();
}
}

@Test
public void testDeleteTrashNoSkipTrash() throws Exception {

Expand Down
4 changes: 2 additions & 2 deletions ...fs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicRootedOzoneClientAdapterImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -882,9 +882,9 @@ public List<FileStatusAdapter> listStatus(String pathStr, boolean recursive,
}
OFSPath ofsStartPath = new OFSPath(startPath, config);
if (ofsPath.isVolume()) {
String startBucket = ofsStartPath.getBucketName();
String startBucketPath = ofsStartPath.getNonKeyPath();
return listStatusVolume(ofsPath.getVolumeName(),
recursive, startBucket, numEntries, uri, workingDir, username);
recursive, startBucketPath, numEntries, uri, workingDir, username);
Comment on lines +885 to +887
Copy link
Contributor

@adoroszlai adoroszlai Mar 22, 2024

Choose a reason for hiding this comment

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

I suggest changing startPath parameter of listStatusVolume to OFSPath, and passing ofsStartPath. This reduces the chance for such misuse, and also avoids creating another instance of OFSPath for the same path in the method. The method may also validate that ofsStartPath.getVolumeName() equals volumeNameStr.

Copy link
Contributor Author

@symious symious Mar 22, 2024

Choose a reason for hiding this comment

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

Currently, all the following methods are using String as parameter, maybe we can align to use String here or it might confuse people more?

listStatusBucketSnapshot(ofsPath.getVolumeName(), ofsPath.getBucketName(), uri);
listStatusVolume(ofsPath.getVolumeName(), recursive, startBucketPath, numEntries, uri, workingDir, username);
listStatusRoot(recursive, startPath, numEntries, uri, workingDir, username);

}

if (ofsPath.isSnapshotPath()) {
Expand Down