jojochuang commented on code in PR #3768:
URL: https://github.com/apache/ozone/pull/3768#discussion_r980408938
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java:
##########
@@ -1690,7 +1691,8 @@ private void findKeyInDbWithIterator(boolean recursive,
String startKey,
if (!entryKeyName.equals(immediateChild)) {
OmKeyInfo fakeDirEntry = createDirectoryKey(
omKeyInfo.getVolumeName(), omKeyInfo.getBucketName(),
- immediateChild, omKeyInfo.getAcls());
+ immediateChild, omKeyInfo.getAcls(),
+ omKeyInfo.getReplicationConfig());
Review Comment:
This is fine.
But if most of the parameters are taken from omKeyInfo, we can consider
making createDirectoryKey() to take omKeyInfo and immediateChild only. That way
if OmKeyInfo adds new fields in the future, we don't need to keep updating it
and its caller.
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/TestDataUtil.java:
##########
@@ -63,7 +61,8 @@ public static OzoneBucket
createVolumeAndBucket(MiniOzoneCluster cluster,
if (bucketLayout != null) {
builder.setBucketLayout(bucketLayout);
}
- omBucketArgs = builder.build();
+ omBucketArgs =
+ builder.setDefaultReplicationConfig(new
DefaultReplicationConfig(new ECReplicationConfig(3,2))).build();
Review Comment:
Unrelated, but it would be nice to have ECReplicationConfig objects as
singletons.
I suspect we'll end up seeing a lot of duplicate ECReplicationConfig objects
in the heap in the future.
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestOzoneFileSystem.java:
##########
@@ -610,12 +602,68 @@ public void testListStatusWithIntermediateDir() throws
Exception {
}
FileStatus[] fileStatuses = fs.listStatus(parent);
-
// the number of immediate children of root is 1
Assert.assertEquals(1, fileStatuses.length);
writeClient.deleteKey(keyArgs);
}
+ @Test
+ public void testListStatusWithIntermediateDirWithECEnabled() throws
Exception {
+ System.out.println("Executing");
+ String keyName = "object-dir/object-name1";
+ OmKeyArgs keyArgs = new OmKeyArgs.Builder()
+ .setVolumeName(volumeName)
+ .setBucketName(bucketName)
+ .setKeyName(keyName)
+ .setAcls(Collections.emptyList())
+ .setReplicationConfig(new ECReplicationConfig(3, 2))
+ .setLocationInfoList(new ArrayList<>())
+ .build();
+ OpenKeySession session = writeClient.openKey(keyArgs);
+
+ writeClient.commitKey(keyArgs, session.getId());
+
+
+
+
+
+
Review Comment:
```suggestion
```
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestOzoneFileSystem.java:
##########
@@ -209,6 +200,7 @@ public static void teardown() {
@After
public void cleanup() {
try {
+ System.out.println("Cleaning Up");
Review Comment:
```suggestion
LOG.info("Cleaning Up");
```
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/TestDataUtil.java:
##########
@@ -23,9 +23,7 @@
import java.util.HashMap;
import java.util.Scanner;
-import org.apache.hadoop.hdds.client.ReplicationConfig;
-import org.apache.hadoop.hdds.client.ReplicationFactor;
-import org.apache.hadoop.hdds.client.ReplicationType;
+import org.apache.hadoop.hdds.client.*;
Review Comment:
This change is not needed.
You can consider updating IntelliJ settings to not make wildcard imports.
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestOzoneFileSystem.java:
##########
@@ -610,12 +602,68 @@ public void testListStatusWithIntermediateDir() throws
Exception {
}
FileStatus[] fileStatuses = fs.listStatus(parent);
-
// the number of immediate children of root is 1
Assert.assertEquals(1, fileStatuses.length);
writeClient.deleteKey(keyArgs);
}
+ @Test
+ public void testListStatusWithIntermediateDirWithECEnabled() throws
Exception {
+ System.out.println("Executing");
+ String keyName = "object-dir/object-name1";
+ OmKeyArgs keyArgs = new OmKeyArgs.Builder()
+ .setVolumeName(volumeName)
+ .setBucketName(bucketName)
+ .setKeyName(keyName)
+ .setAcls(Collections.emptyList())
+ .setReplicationConfig(new ECReplicationConfig(3, 2))
+ .setLocationInfoList(new ArrayList<>())
+ .build();
+ OpenKeySession session = writeClient.openKey(keyArgs);
+
+ writeClient.commitKey(keyArgs, session.getId());
+
+
+
+
+
+
+
+ keyName = "object-dir/object-name2";
+ keyArgs = new OmKeyArgs.Builder()
+ .setVolumeName(volumeName)
+ .setBucketName(bucketName)
+ .setKeyName(keyName)
+ .setAcls(Collections.emptyList())
+ .setReplicationConfig(new ECReplicationConfig(3, 2))
+ .setLocationInfoList(new ArrayList<>())
+ .build();
+ session = writeClient.openKey(keyArgs);
+
+ writeClient.commitKey(keyArgs, session.getId());
+
+ Path parent = new Path("/");
+
+ // Wait until the filestatus is updated
+ if (!enabledFileSystemPaths) {
+ GenericTestUtils.waitFor(() -> {
+ try {
+ return fs.listStatus(parent).length != 0;
+ } catch (IOException e) {
+ LOG.error("listStatus() Failed", e);
+ Assert.fail("listStatus() Failed");
+ return false;
+ }
+ }, 1000, 120000);
+ }
Review Comment:
is this necessary? I am under the impression commitKey() is synchronous and
listStatus() should reflect the change immediately?
--
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]
