adoroszlai commented on code in PR #9750:
URL: https://github.com/apache/ozone/pull/9750#discussion_r2794096031
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/OzoneFileSystemTestBase.java:
##########
@@ -39,35 +36,24 @@
import org.apache.hadoop.fs.Path;
import org.apache.hadoop.fs.RemoteIterator;
import org.apache.hadoop.fs.contract.ContractTestUtils;
-import org.apache.hadoop.hdds.conf.ConfigurationTarget;
import org.apache.hadoop.hdds.conf.OzoneConfiguration;
-import org.apache.ratis.util.Preconditions;
+import org.apache.hadoop.hdds.protocol.proto.HddsProtos;
+import org.apache.hadoop.ozone.client.OzoneKeyDetails;
/**
* Common test cases for Ozone file systems.
*/
-public final class OzoneFileSystemTests {
+public abstract class OzoneFileSystemTestBase {
- private OzoneFileSystemTests() {
+ protected OzoneFileSystemTestBase() {
// no instances
}
Review Comment:
The comment is no longer valid. The constructor can be completely removed,
we can rely on the default one.
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/OzoneFileSystemTestBase.java:
##########
@@ -39,35 +36,24 @@
import org.apache.hadoop.fs.Path;
import org.apache.hadoop.fs.RemoteIterator;
import org.apache.hadoop.fs.contract.ContractTestUtils;
-import org.apache.hadoop.hdds.conf.ConfigurationTarget;
import org.apache.hadoop.hdds.conf.OzoneConfiguration;
-import org.apache.ratis.util.Preconditions;
+import org.apache.hadoop.hdds.protocol.proto.HddsProtos;
+import org.apache.hadoop.ozone.client.OzoneKeyDetails;
/**
* Common test cases for Ozone file systems.
*/
-public final class OzoneFileSystemTests {
+public abstract class OzoneFileSystemTestBase {
- private OzoneFileSystemTests() {
+ protected OzoneFileSystemTestBase() {
// no instances
}
- /**
- * Set file system listing page size. Also disable the file system cache to
- * ensure new {@link FileSystem} instance reflects the updated page size.
- */
- public static void setPageSize(ConfigurationTarget conf, int pageSize) {
- Preconditions.assertTrue(pageSize > 0, () -> "pageSize=" + pageSize + " <=
0");
- conf.setInt(OZONE_FS_LISTING_PAGE_SIZE, pageSize);
- conf.setBoolean(String.format("fs.%s.impl.disable.cache",
OZONE_URI_SCHEME), true);
- conf.setBoolean(String.format("fs.%s.impl.disable.cache",
OZONE_OFS_URI_SCHEME), true);
- }
-
/**
* Tests listStatusIterator operation on directory with different
* numbers of child directories.
*/
- public static void listStatusIteratorOnPageSize(OzoneConfiguration conf,
+ protected void listStatusIteratorOnPageSize(OzoneConfiguration conf,
Review Comment:
This and other existing methods that are not going to be overridden can
still be `static`.
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/AbstractOzoneFileSystemTest.java:
##########
@@ -977,54 +961,12 @@ public void testListStatusOnSubDirs() throws Exception {
*/
@Test
public void testListStatusIteratorWithDir() throws Exception {
- Path parent = new Path(ROOT, "testListStatus");
- Path file1 = new Path(parent, "key1");
- Path file2 = new Path(parent, "key2");
- try {
- // Iterator should have no items when dir is empty
- RemoteIterator<FileStatus> it = o3fs.listStatusIterator(ROOT);
- assertFalse(it.hasNext());
-
- ContractTestUtils.touch(fs, file1);
- ContractTestUtils.touch(fs, file2);
- // Iterator should have an item when dir is not empty
- it = o3fs.listStatusIterator(ROOT);
- while (it.hasNext()) {
- FileStatus fileStatus = it.next();
- assertNotNull(fileStatus);
- assertEquals(fileStatus.getPath().toUri().getPath(),
parent.toString(), "Parent path doesn't match");
- }
- // Iterator on a directory should return all subdirs along with
- // files, even if there exists a file and sub-dir with the same name.
- it = o3fs.listStatusIterator(parent);
- int iCount = 0;
- while (it.hasNext()) {
- iCount++;
- FileStatus fileStatus = it.next();
- assertNotNull(fileStatus);
- }
- assertEquals(2, iCount, "Iterator did not return all the file status");
- // Iterator should return file status for only the
- // immediate children of a directory.
- Path file3 = new Path(parent, "dir1/key3");
- Path file4 = new Path(parent, "dir1/key4");
- ContractTestUtils.touch(fs, file3);
- ContractTestUtils.touch(fs, file4);
- it = o3fs.listStatusIterator(parent);
- iCount = 0;
-
- while (it.hasNext()) {
- iCount++;
- FileStatus fileStatus = it.next();
- assertNotNull(fileStatus);
- }
- assertEquals(3, iCount, "Iterator did not return file status " +
- "of all the children of the directory");
+ listStatusIteratorWithDir(ROOT, fs);
+ }
- } finally {
- // Cleanup
- fs.delete(parent, true);
- }
+ @Override
+ RemoteIterator<FileStatus> listStatusIterator(Path path) throws IOException {
+ return o3fs.listStatusIterator(path);
Review Comment:
Let's define `public abstract FileSystem getFs()` in the parent class
(implementations already exist, just add `@Override`). This way we don't need
to add a template method for each `FileSystem` operation (like
`listStatusIterator(Path)`) in the tests.
--
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]
