errose28 commented on code in PR #3953:
URL: https://github.com/apache/ozone/pull/3953#discussion_r1029790780
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestOzoneFileSystemWithFSO.java:
##########
@@ -348,6 +349,32 @@ public void testRenameDestinationParentDoesntExist()
throws Exception {
Assert.assertFalse(getFs().rename(dir2SourcePath, newDestinPath));
}
+ @Test
+ public void testRenameParentDirModificationTime() throws IOException {
Review Comment:
This tests rename of a key from one directory to another. It would be good
to also test rename of a directory from one parent directory to another.
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyRenameRequestWithFSO.java:
##########
@@ -18,22 +18,78 @@
package org.apache.hadoop.ozone.om.request.key;
+import org.apache.hadoop.fs.Path;
+import org.apache.hadoop.hdds.protocol.proto.HddsProtos;
import org.apache.hadoop.ozone.OmUtils;
import org.apache.hadoop.ozone.OzoneConsts;
+import org.apache.hadoop.ozone.om.exceptions.OMException;
import org.apache.hadoop.ozone.om.helpers.BucketLayout;
+import org.apache.hadoop.ozone.om.helpers.OmDirectoryInfo;
+import org.apache.hadoop.ozone.om.helpers.OmKeyInfo;
+import org.apache.hadoop.ozone.om.request.file.OMFileRequest;
+import org.apache.hadoop.ozone.om.request.OMRequestTestUtils;
import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos;
import
org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.KeyArgs;
import
org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.OMRequest;
import
org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.RenameKeyRequest;
+import org.apache.hadoop.util.Time;
import org.junit.Assert;
+import org.junit.Before;
import org.junit.Test;
+import java.io.IOException;
import java.util.UUID;
/**
* Tests RenameKeyWithFSO request.
*/
-public class TestOMKeyRenameRequestWithFSO extends TestOMKeyRequest {
+public class TestOMKeyRenameRequestWithFSO extends TestOMKeyRenameRequest {
+ private OmKeyInfo formKeyParentInfo;
Review Comment:
Same `form` -> `from` typo here as above. Looks like it is also present in
local variables below.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyRenameRequestWithFSO.java:
##########
@@ -243,27 +244,56 @@ public OMClientResponse
validateAndUpdateCache(OzoneManager ozoneManager,
}
@SuppressWarnings("parameternumber")
- private OMClientResponse renameKey(long toKeyParentId,
- long trxnLogIndex, OmKeyInfo fromKeyValue, boolean isRenameDirectory,
- String toKeyName, long modificationTime, OMResponse.Builder omResponse,
- OzoneManager ozoneManager) throws IOException {
-
+ private OMClientResponse renameKey(OmKeyInfo toKeyParent, String toKeyName,
+ OmKeyInfo fromKeyValue, String fromKeyName, boolean isRenameDirectory,
+ long modificationTime, OzoneManager ozoneManager,
+ OMResponse.Builder omResponse, long trxnLogIndex) throws IOException {
final OMMetadataManager ommm = ozoneManager.getMetadataManager();
final long volumeId = ommm.getVolumeId(fromKeyValue.getVolumeName());
final long bucketId = ommm.getBucketId(fromKeyValue.getVolumeName(),
fromKeyValue.getBucketName());
final String dbFromKey = ommm.getOzonePathKey(volumeId, bucketId,
fromKeyValue.getParentObjectID(), fromKeyValue.getFileName());
String toKeyFileName = OzoneFSUtils.getFileName(toKeyName);
+ OmKeyInfo fromKeyParent = null;
+ OMMetadataManager metadataMgr = ozoneManager.getMetadataManager();
+ Table<String, OmDirectoryInfo> dirTable = metadataMgr.getDirectoryTable();
fromKeyValue.setUpdateID(trxnLogIndex, ozoneManager.isRatisEnabled());
// Set toFileName
fromKeyValue.setKeyName(toKeyFileName);
fromKeyValue.setFileName(toKeyFileName);
// Set toKeyObjectId
- fromKeyValue.setParentObjectID(toKeyParentId);
- //Set modification time
- fromKeyValue.setModificationTime(modificationTime);
+ if (toKeyParent != null) {
+ fromKeyValue.setParentObjectID(toKeyParent.getObjectID());
+ } else {
+ String bucketKey = metadataMgr.getBucketKey(
+ fromKeyValue.getVolumeName(), fromKeyValue.getBucketName());
+ OmBucketInfo omBucketInfo =
+ metadataMgr.getBucketTable().get(bucketKey);
+ fromKeyValue.setParentObjectID(omBucketInfo.getObjectID());
Review Comment:
Do we want a rename immediately under the bucket to update the bucket's
modification time? If yes, then `KeyRenameRequest` should be modified for
consistent behavior on object store buckets, the corresponding response updated
to flush the bucket to the DB, and tests added.
I'm not sure which option is better, but we should make a definitive call
here. For context it looks like we are hardly ever setting bucket modification
time except for changing the owner.
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyRenameRequest.java:
##########
@@ -37,26 +40,37 @@
/**
* Tests RenameKey request.
*/
+@SuppressWarnings("checkstyle:VisibilityModifier")
public class TestOMKeyRenameRequest extends TestOMKeyRequest {
+ protected OmKeyInfo formKeyInfo;
+ protected String formKeyName;
+ protected String toKeyName;
+ protected String dbToKey;
+
+ @Before
+ public void createParentKey() throws Exception {
+ OMRequestTestUtils.addVolumeAndBucketToDB(volumeName, bucketName,
+ omMetadataManager, getBucketLayout());
+ formKeyName = new Path("formKey").toString();
+ toKeyName = new Path("toKey").toString();
+ formKeyInfo = getOmKeyInfo(formKeyName);
Review Comment:
nit. `form` should be `from` in these variable names.
--
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]
