You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "dombizita (via GitHub)" <gi...@apache.org> on 2023/03/27 10:04:01 UTC
[GitHub] [ozone] dombizita opened a new pull request, #4478: HDDS-8242. Rename operation not working with FSO bucket destination
dombizita opened a new pull request, #4478:
URL: https://github.com/apache/ozone/pull/4478
## What changes were proposed in this pull request?
Currently if we want to move (with rename) a key to bucket level (eg. from `/vol1/buck1/dir1/key1` to `/vol1/buck1/key1`) and the bucket is FSO it'll fail with `Invalid KeyPath, empty keyName`. The problem is that if the destination path does not contain the key name it won't add the source key name to the path (like it does if the rename is happening inside a non FSO bucket).
The patch adds this functionality to the `renameFSO()` method and to have the key name I added a new `getFileName()` method to the `OFSPath` class, which will give back the leaf key.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-8242
## How was this patch tested?
Added unit tests and successful CI on my fork: https://github.com/dombizita/ozone/actions/runs/4510369572
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] dombizita commented on a diff in pull request #4478: HDDS-8242. Rename operation not working with FSO bucket destination
Posted by "dombizita (via GitHub)" <gi...@apache.org>.
dombizita commented on code in PR #4478:
URL: https://github.com/apache/ozone/pull/4478#discussion_r1156003371
##########
hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicRootedOzoneFileSystem.java:
##########
@@ -430,8 +430,14 @@ private boolean renameFSO(OzoneBucket bucket,
// construct src and dst key paths
String srcKeyPath = srcPath.getNonKeyPathNoPrefixDelim() +
OZONE_URI_DELIMITER + srcPath.getKeyName();
- String dstKeyPath = dstPath.getNonKeyPathNoPrefixDelim() +
- OZONE_URI_DELIMITER + dstPath.getKeyName();
+ String dstKeyPath;
+ if (dstPath.isBucket()) {
+ dstKeyPath = dstPath.getNonKeyPathNoPrefixDelim() +
+ OZONE_URI_DELIMITER + srcPath.getFileName();
+ } else {
+ dstKeyPath = dstPath.getNonKeyPathNoPrefixDelim() +
+ OZONE_URI_DELIMITER + dstPath.getKeyName();
Review Comment:
> For OMKeyRenamRequest, we should not allow empty key name, that part is ok. Only for OMKeyRenameRequestWithFSO, can remove toKeyName.length() == 0 check, and any further check if causing issue.
@sumitagrawl why do you think that we should differentiate the way we handle FSO and non FSO buckets during the rename of a key/directory?
> I am proposing to fix at OM layer, because other client integration do not need any change when passing source and destination, and OM layer should support api with this behaviour.
I don't think so, the rename in case of a non FSO bucket is also doing the necessary changes in the destination path (in both implementations in `BasicOzoneFilesystem` and `BasicRootedOzoneFilesystem`). I believe it wasn't intentional that in case of a non FSO bucket we didn't do the change.
as a conclusion, I'd like to avoid to remove the key name check from the `OMKeyRenameRequestWithFSO` as it would cause inconsistency (with `OMKeyRenameRequest`), which is not necessary in this case and it can be confusing. let me know what you think of it.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] dombizita commented on a diff in pull request #4478: HDDS-8242. Rename operation not working with FSO bucket destination
Posted by "dombizita (via GitHub)" <gi...@apache.org>.
dombizita commented on code in PR #4478:
URL: https://github.com/apache/ozone/pull/4478#discussion_r1152071974
##########
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/OFSPath.java:
##########
@@ -163,6 +163,14 @@ public String getKeyName() {
return keyName;
}
+ /**
+ * Returns the last element of the KeyName, which can either be a file name
+ * or the leaf directory/bucket.
+ */
+ public String getFileName() {
+ return keyName.substring(keyName.lastIndexOf(OZONE_URI_DELIMITER) + 1);
Review Comment:
thanks @sadanand48, this is a better way to check it, my approach was even not correct in the case of a path like `/vol1/bucket1/dir1/` as it returned empty. I updated my patch accordingly and also adjusted 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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sadanand48 commented on a diff in pull request #4478: HDDS-8242. Rename operation not working with FSO bucket destination
Posted by "sadanand48 (via GitHub)" <gi...@apache.org>.
sadanand48 commented on code in PR #4478:
URL: https://github.com/apache/ozone/pull/4478#discussion_r1151554904
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestRootedOzoneFileSystemWithFSO.java:
##########
@@ -137,6 +137,19 @@ public void testRenameDestinationParentDoesntExist() throws Exception {
Assert.assertFalse(getFs().rename(dir2SourcePath, newDestinPath));
}
+ @Test
+ public void testKeyToBucketLevel() throws IOException {
Review Comment:
nit : `testKeyRenameToBucketLevel`
##########
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/OFSPath.java:
##########
@@ -163,6 +163,14 @@ public String getKeyName() {
return keyName;
}
+ /**
+ * Returns the last element of the KeyName, which can either be a file name
+ * or the leaf directory/bucket.
+ */
+ public String getFileName() {
+ return keyName.substring(keyName.lastIndexOf(OZONE_URI_DELIMITER) + 1);
Review Comment:
Could use Paths.getFilename() instead
```suggestion
return Paths.get(keyName).getFileName().toString();
```
##########
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/OFSPath.java:
##########
@@ -163,6 +163,14 @@ public String getKeyName() {
return keyName;
}
+ /**
+ * Returns the last element of the KeyName, which can either be a file name
+ * or the leaf directory/bucket.
+ */
+ public String getFileName() {
Review Comment:
This can be a directory too i.e `mv vol1/buck1/dir1/dir2 vol1/buck1/`
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] jojochuang commented on a diff in pull request #4478: HDDS-8242. Rename operation not working with FSO bucket destination
Posted by "jojochuang (via GitHub)" <gi...@apache.org>.
jojochuang commented on code in PR #4478:
URL: https://github.com/apache/ozone/pull/4478#discussion_r1150822718
##########
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/OFSPath.java:
##########
@@ -163,6 +163,14 @@ public String getKeyName() {
return keyName;
}
+ /**
+ * Returns the last element of the KeyName, which can either be a file name
+ * or the leaf directory/bucket.
Review Comment:
is this description accurate? if path = "/vol/buc2", keyName is (empty) and fileName is (empty). It won't be a bucket name.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] dombizita commented on a diff in pull request #4478: HDDS-8242. Rename operation not working with FSO bucket destination
Posted by "dombizita (via GitHub)" <gi...@apache.org>.
dombizita commented on code in PR #4478:
URL: https://github.com/apache/ozone/pull/4478#discussion_r1152074245
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestRootedOzoneFileSystemWithFSO.java:
##########
@@ -137,6 +137,19 @@ public void testRenameDestinationParentDoesntExist() throws Exception {
Assert.assertFalse(getFs().rename(dir2SourcePath, newDestinPath));
}
+ @Test
+ public void testKeyToBucketLevel() throws IOException {
Review Comment:
thanks, I changed it
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] fapifta commented on a diff in pull request #4478: HDDS-8242. Rename operation not working with FSO bucket destination
Posted by "fapifta (via GitHub)" <gi...@apache.org>.
fapifta commented on code in PR #4478:
URL: https://github.com/apache/ozone/pull/4478#discussion_r1163365031
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyRenameRequestWithFSO.java:
##########
@@ -101,7 +102,7 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
OmKeyInfo fromKeyValue;
Result result;
try {
- if (toKeyName.length() == 0 || fromKeyName.length() == 0) {
+ if (fromKeyName.length() == 0) {
throw new OMException("Key name is empty",
Review Comment:
After changing the condition, I believe we can be more precise in the message and denote that the source key is the problematic one.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sumitagrawl commented on a diff in pull request #4478: HDDS-8242. Rename operation not working with FSO bucket destination
Posted by "sumitagrawl (via GitHub)" <gi...@apache.org>.
sumitagrawl commented on code in PR #4478:
URL: https://github.com/apache/ozone/pull/4478#discussion_r1156932884
##########
hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicRootedOzoneFileSystem.java:
##########
@@ -430,8 +430,14 @@ private boolean renameFSO(OzoneBucket bucket,
// construct src and dst key paths
String srcKeyPath = srcPath.getNonKeyPathNoPrefixDelim() +
OZONE_URI_DELIMITER + srcPath.getKeyName();
- String dstKeyPath = dstPath.getNonKeyPathNoPrefixDelim() +
- OZONE_URI_DELIMITER + dstPath.getKeyName();
+ String dstKeyPath;
+ if (dstPath.isBucket()) {
+ dstKeyPath = dstPath.getNonKeyPathNoPrefixDelim() +
+ OZONE_URI_DELIMITER + srcPath.getFileName();
+ } else {
+ dstKeyPath = dstPath.getNonKeyPathNoPrefixDelim() +
+ OZONE_URI_DELIMITER + dstPath.getKeyName();
Review Comment:
@dombizita yes, non-FSO can not have destination as empty key for bucket, key is present at bucket level. So non-FSO key length can not be "0" size.
While FSO, rename also simulate mv of file from deep directory to bucket level also. So here target keyname size can be "0" and take source filename here. So there is a bug in OM that it does not allow target with just bucket, and as suggested can remove the check in Rename FSO case.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] fapifta commented on a diff in pull request #4478: HDDS-8242. Rename operation not working with FSO bucket destination
Posted by "fapifta (via GitHub)" <gi...@apache.org>.
fapifta commented on code in PR #4478:
URL: https://github.com/apache/ozone/pull/4478#discussion_r1157027665
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestRootedOzoneFileSystemWithFSO.java:
##########
@@ -137,6 +137,19 @@ public void testRenameDestinationParentDoesntExist() throws Exception {
Assert.assertFalse(getFs().rename(dir2SourcePath, newDestinPath));
}
+ @Test
+ public void testKeyRenameToBucketLevel() throws IOException {
+ final String dir = "dir1";
+ final String key = dir + "/key1";
+ final Path source = new Path(getBucketPath(), key);
+ final Path dest = new Path(String.valueOf(getBucketPath()));
+ LOG.info("Will move {} to {}", source, dest);
+ getFs().rename(source, dest);
+ assertTrue("Key rename failed", getFs().exists(dest));
Review Comment:
Here we are asserting if dest exists, but dest is the bucket path, which I believe should always yield to true.
The intent I guess would be to check whether dest/key1 exists, or do I misunderstand something?
##########
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/OFSPath.java:
##########
@@ -163,6 +163,14 @@ public String getKeyName() {
return keyName;
}
+ /**
+ * Returns the last element of the KeyName, which can either be a file name
+ * or the leaf directory/bucket.
+ */
+ public String getFileName() {
Review Comment:
Maybe getLastElement()?
##########
hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicRootedOzoneFileSystem.java:
##########
@@ -430,8 +430,14 @@ private boolean renameFSO(OzoneBucket bucket,
// construct src and dst key paths
String srcKeyPath = srcPath.getNonKeyPathNoPrefixDelim() +
OZONE_URI_DELIMITER + srcPath.getKeyName();
- String dstKeyPath = dstPath.getNonKeyPathNoPrefixDelim() +
- OZONE_URI_DELIMITER + dstPath.getKeyName();
+ String dstKeyPath;
+ if (dstPath.isBucket()) {
+ dstKeyPath = dstPath.getNonKeyPathNoPrefixDelim() +
+ OZONE_URI_DELIMITER + srcPath.getFileName();
+ } else {
+ dstKeyPath = dstPath.getNonKeyPathNoPrefixDelim() +
+ OZONE_URI_DELIMITER + dstPath.getKeyName();
Review Comment:
@sumitagrawl, @dombizita
Let me chime in a bit into this discussion and think about this loud.
The basic problem is this as I understand:
During a rename operation, where we need to resolve the target's final path and name in case the target of the rename is a directory, and especially when the target is a bucket.
Clearly the expectation is coming from the semantics of how a file system rename operation behaves in the local filesystem world, as we are not different from any local filesystem when it comes to operations with files, but we are mainly an object store that is designed to be accessible as a file system as well.
HDFS solves this clearly just on the NameNode side, while GNU mv for example solves this internally within the command and does not push this responsibility to the filesystem implementation. So both schools exists.
We have 3 bucket types, legacy OBS and FSO, but some of the codebase is different for these on the client and the server side, let's see what we have internally:
- preExecute code is common for all three bucket types.
- legacy and OBS request is handled by the OmKeyRenameRequest class, FSO is handled by OmKeyRenameRequestWithFSO class
- in preExecute we have a check via the validateAndNormalizeKey(boolean, String, BucketLayout) method, this method operates only if the bucket type is FSO, or if the bucket type is legacy and "ozone.om.enable.filesystem.paths" is enabled (default is disabled). Normalization on empty keyName does nothing with the keyName, and then validation throws an exception as an empty key name is invalid.
- the actual request handling in validateAndUpdateCache later on for legacy bucket type checks if the target keyName's length is greater than zero, if not, it throws an exception and denies the request, similarly the FSO bucket type's request handling denies the request with an exception being thrown if the target keyName's lenght is not greater than zero.
What needs to be done on the client side to fix this problem is showcased in this patch by @dombizita. It is fairly simple, and definitely simpler then what I can imagine on the server side, but, Let me play around with the thought what would it take to do the fix there.
Considering the current class structure, as OBS and legacy are handled together with the same code, and FSO is the only one that has a specialization, our solution either separates OBS and legacy from each other, or the two solution is tied together. I think it is not feasible to separate OBS and legacy for this, as the problem does not manifests with these bucket types at all. But why? Because this case is solved for legacy buckets on the client side. How? In BasicOzoneFileSystem and BasicRootedOzoneFilesystem it is handled, and for the RenameKeyHandler in the object store CLI client it is not allowed to have an empty target key name.
Ok, so legacy and OBS are handled on the client side, and we do not need to care... What we can do with FSO then on the server side?
We can specialize the preExecute's validation on the target key, and allow an empty target keyName to go through. What would that imply? In the preexecute it is not a problem, we can introduce a method that abstracts the validation away for the target path, and allow empty key name only in FSO. Fairly simple so far.
Let's see what it takes to allow empty target in validateAndUpdateCache... Only for FSO after preExecute logic is changed.
In the check for keyName lengths we can omit the check for toKeyName, easy.
After that ACL checks are the next step with an empty key name I think we can safely say it is undefined what we will get back, so we are forced to form the proper target key before ACL checks are happening.
How we can do that? We can put an if check there, and see if the target key name length is zero, and if it is, then we need to check the ACLs on the bucket, and not on the key for the target. One additional branching add for complexity.
verifyToDirIsASubDirOfFromDirectory should work with empty target key name, easy
getOmKeyInfoIfExists should return null for the target file status
It seems that after this the renameKey call handles the case when the target parent is a bucket, and does the rename correctly by looking at the code.
Based on this I think even if it causes some more hurdle to handle this on the server side, it is not that big of a difference (if I am right about what is necessary) considering confusion that the mixed solution may cause when we have parts of the solution on the client side while other parts on the server side. It already causes confusion that legacy and OBS is solved on the client side, while FSO is on the server side, but we can try to keep consistency with this approach.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sumitagrawl commented on a diff in pull request #4478: HDDS-8242. Rename operation not working with FSO bucket destination
Posted by "sumitagrawl (via GitHub)" <gi...@apache.org>.
sumitagrawl commented on code in PR #4478:
URL: https://github.com/apache/ozone/pull/4478#discussion_r1151697474
##########
hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicRootedOzoneFileSystem.java:
##########
@@ -430,8 +430,14 @@ private boolean renameFSO(OzoneBucket bucket,
// construct src and dst key paths
String srcKeyPath = srcPath.getNonKeyPathNoPrefixDelim() +
OZONE_URI_DELIMITER + srcPath.getKeyName();
- String dstKeyPath = dstPath.getNonKeyPathNoPrefixDelim() +
- OZONE_URI_DELIMITER + dstPath.getKeyName();
+ String dstKeyPath;
+ if (dstPath.isBucket()) {
+ dstKeyPath = dstPath.getNonKeyPathNoPrefixDelim() +
+ OZONE_URI_DELIMITER + srcPath.getFileName();
+ } else {
+ dstKeyPath = dstPath.getNonKeyPathNoPrefixDelim() +
+ OZONE_URI_DELIMITER + dstPath.getKeyName();
Review Comment:
dstKeyPath if its bucket, is already handled in OM side adding filename. Having similar logic in CLI part may not be required. Can check why its failing at OM. IMO, we can fix at OM for this case.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] kerneltime commented on pull request #4478: HDDS-8242. Rename operation not working with FSO bucket destination
Posted by "kerneltime (via GitHub)" <gi...@apache.org>.
kerneltime commented on PR #4478:
URL: https://github.com/apache/ozone/pull/4478#issuecomment-1485415178
cc @swamirishi
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sumitagrawl commented on pull request #4478: HDDS-8242. Rename operation not working with FSO bucket destination
Posted by "sumitagrawl (via GitHub)" <gi...@apache.org>.
sumitagrawl commented on PR #4478:
URL: https://github.com/apache/ozone/pull/4478#issuecomment-1498936301
@fapifta @dombizita I have tried below changes, and its working, other changes seems not required,
--- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyRenameRequest.java
+++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyRenameRequest.java
@@ -97,8 +97,10 @@ public OMRequest preExecute(OzoneManager ozoneManager) throws IOException {
if (getBucketLayout().isFileSystemOptimized()) {
srcKey = validateAndNormalizeKey(ozoneManager.getEnableFileSystemPaths(),
srcKey, getBucketLayout());
- dstKey = validateAndNormalizeKey(ozoneManager.getEnableFileSystemPaths(),
- dstKey, getBucketLayout());
+ if (!dstKey.isEmpty()) {
+ dstKey = validateAndNormalizeKey(ozoneManager.getEnableFileSystemPaths(),
+ dstKey, getBucketLayout());
+ }
}
// Set modification time & srcKeyName.
diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyRenameRequestWithFSO.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyRenameRequestWithFSO.java
index 0220d1bf0..6b480773d 100644
--- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyRenameRequestWithFSO.java
+++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyRenameRequestWithFSO.java
@@ -100,7 +100,7 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
OmKeyInfo fromKeyValue;
Result result;
try {
- if (toKeyName.length() == 0 || fromKeyName.length() == 0) {
+ if (fromKeyName.length() == 0) {
throw new OMException("Key name is empty",
OMException.ResultCodes.INVALID_KEY_NAME);
}
@@ -199,10 +199,18 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
// destination path.
OmKeyInfo toKeyParent = OMFileRequest.getKeyParentDir(volumeName,
bucketName, toKeyName, omMetadataManager);
-
- omClientResponse = renameKey(toKeyParent, toKeyName, fromKeyValue,
- fromKeyName, isRenameDirectory, keyArgs.getModificationTime(),
- ozoneManager, omResponse, trxnLogIndex);
+ if (null == toKeyParent) {
+ String fromFileName = OzoneFSUtils.getFileName(fromKeyName);
+ String newToKeyName = OzoneFSUtils.appendFileNameToKeyPath(toKeyName,
+ fromFileName);
+ omClientResponse = renameKey(toKeyParent, newToKeyName, fromKeyValue,
+ fromKeyName, isRenameDirectory, keyArgs.getModificationTime(),
+ ozoneManager, omResponse, trxnLogIndex);
+ } else {
+ omClientResponse = renameKey(toKeyParent, toKeyName, fromKeyValue,
+ fromKeyName, isRenameDirectory, keyArgs.getModificationTime(),
+ ozoneManager, omResponse, trxnLogIndex);
+ }
result = Result.SUCCESS;
}
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sumitagrawl commented on a diff in pull request #4478: HDDS-8242. Rename operation not working with FSO bucket destination
Posted by "sumitagrawl (via GitHub)" <gi...@apache.org>.
sumitagrawl commented on code in PR #4478:
URL: https://github.com/apache/ozone/pull/4478#discussion_r1152760162
##########
hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicRootedOzoneFileSystem.java:
##########
@@ -430,8 +430,14 @@ private boolean renameFSO(OzoneBucket bucket,
// construct src and dst key paths
String srcKeyPath = srcPath.getNonKeyPathNoPrefixDelim() +
OZONE_URI_DELIMITER + srcPath.getKeyName();
- String dstKeyPath = dstPath.getNonKeyPathNoPrefixDelim() +
- OZONE_URI_DELIMITER + dstPath.getKeyName();
+ String dstKeyPath;
+ if (dstPath.isBucket()) {
+ dstKeyPath = dstPath.getNonKeyPathNoPrefixDelim() +
+ OZONE_URI_DELIMITER + srcPath.getFileName();
+ } else {
+ dstKeyPath = dstPath.getNonKeyPathNoPrefixDelim() +
+ OZONE_URI_DELIMITER + dstPath.getKeyName();
Review Comment:
@dombizita For OMKeyRenamRequest, we should not allow empty key name, that part is ok. Only for OMKeyRenameRequestWithFSO, can remove toKeyName.length() == 0 check, and any further check if causing issue.
I am proposing to fix at OM layer, because other client integration do not need any change when passing source and destination, and OM layer should support api with this behavior.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai commented on pull request #4478: HDDS-8242. Rename operation not working with FSO bucket destination
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #4478:
URL: https://github.com/apache/ozone/pull/4478#issuecomment-1509720613
Thanks @dombizita for the patch, @fapifta, @jojochuang, @sadanand48, @sumitagrawl for the review.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] dombizita commented on pull request #4478: HDDS-8242. Rename operation not working with FSO bucket destination
Posted by "dombizita (via GitHub)" <gi...@apache.org>.
dombizita commented on PR #4478:
URL: https://github.com/apache/ozone/pull/4478#issuecomment-1496357296
just summarise the discussion a bit @sumitagrawl. so to be honest I have cons for both of the approaches.
with my approach I don't like that previously there were no relevant checks on the client side (in the `BasicRootedOzoneFilesystem`s `renameFSO()` method) for the FSO buckets, only on the OM side in the `OMKeyRenameRequestWithFSO` class. also with my current changes I just handle if the destination is a bucket and then add the source key name, in case of a directory destination path I just pass it on (so it is not consistent) as it will be a valid path and the source key name will be added in the OM side.
in case we do changes in the `OMKeyRenameRequestWithFSO` we need more changes that I wanted to avoid. first we need to either add a `preExecute()` method for the FSO implementation or remove/move the `validateAndNormalizeKey` to somewhere later in the code. second we need to remove the `toKeyName.length() == 0` check from the `validateAndUpdateCache()` method, but there are a few places where we use the empty `toKeyName` before we handle if the destination path is a directory and add the source key name. so there needs to be done a few changes which can affect other behaviours.
as I looked into this more, your solution would be more consistent with the current code, but I think that these kind of checks should have happen consistently at the same place in both bucket types, as I think nothing explains why we do it differently. but that is already the way it is :)
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] dombizita commented on a diff in pull request #4478: HDDS-8242. Rename operation not working with FSO bucket destination
Posted by "dombizita (via GitHub)" <gi...@apache.org>.
dombizita commented on code in PR #4478:
URL: https://github.com/apache/ozone/pull/4478#discussion_r1157122410
##########
hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicRootedOzoneFileSystem.java:
##########
@@ -430,8 +430,14 @@ private boolean renameFSO(OzoneBucket bucket,
// construct src and dst key paths
String srcKeyPath = srcPath.getNonKeyPathNoPrefixDelim() +
OZONE_URI_DELIMITER + srcPath.getKeyName();
- String dstKeyPath = dstPath.getNonKeyPathNoPrefixDelim() +
- OZONE_URI_DELIMITER + dstPath.getKeyName();
+ String dstKeyPath;
+ if (dstPath.isBucket()) {
+ dstKeyPath = dstPath.getNonKeyPathNoPrefixDelim() +
+ OZONE_URI_DELIMITER + srcPath.getFileName();
+ } else {
+ dstKeyPath = dstPath.getNonKeyPathNoPrefixDelim() +
+ OZONE_URI_DELIMITER + dstPath.getKeyName();
Review Comment:
with this logic also the non FSO destinations can have an empty key name, as the path can also be `vol/buck/dir1/dir2/key` and the destination can be `/vol/buck` too. that is why there is this part in `BasicRootedOzoneFileSystem` in the `rename()` method, to avoid that case and that is why it is not failing in a case like that in the `OMKeyRenameRequest`.
```
else if (dstStatus.isDirectory()) {
// If dst is a directory, rename source as subpath of it.
// for example rename /source to /dst will lead to /dst/source
dst = new Path(dst, src.getName());
FileStatus[] statuses;
try {
statuses = listStatus(dst);
} catch (FileNotFoundException fnde) {
statuses = null;
}
if (statuses != null && statuses.length > 0) {
// If dst exists and not a directory not empty
LOG.warn("Failed to rename {} to {}, file already exists" +
" or not empty!", src, dst);
return false;
}
```
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] dombizita commented on a diff in pull request #4478: HDDS-8242. Rename operation not working with FSO bucket destination
Posted by "dombizita (via GitHub)" <gi...@apache.org>.
dombizita commented on code in PR #4478:
URL: https://github.com/apache/ozone/pull/4478#discussion_r1152061324
##########
hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicRootedOzoneFileSystem.java:
##########
@@ -430,8 +430,14 @@ private boolean renameFSO(OzoneBucket bucket,
// construct src and dst key paths
String srcKeyPath = srcPath.getNonKeyPathNoPrefixDelim() +
OZONE_URI_DELIMITER + srcPath.getKeyName();
- String dstKeyPath = dstPath.getNonKeyPathNoPrefixDelim() +
- OZONE_URI_DELIMITER + dstPath.getKeyName();
+ String dstKeyPath;
+ if (dstPath.isBucket()) {
+ dstKeyPath = dstPath.getNonKeyPathNoPrefixDelim() +
+ OZONE_URI_DELIMITER + srcPath.getFileName();
+ } else {
+ dstKeyPath = dstPath.getNonKeyPathNoPrefixDelim() +
+ OZONE_URI_DELIMITER + dstPath.getKeyName();
Review Comment:
thanks for the review @sumitagrawl, I tried to approach the problem from this perspective and let me summarise what I found.
if I don't handle the destination path key name here, I'll need to handle that it can be empty in case of a bucket path (eg. `/vol1/bucket1`). for that I need to remove/rethink conditions at two places.
1. once in `OMKeyRenameRequest`s `preExecute()` method, where on [line 100](https://github.com/apache/ozone/blob/master/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyRenameRequest.java#L100) we are calling `validateAndNormalizeKey()`. we shouldn't validate (and normalize) the key in case it is empty.
2. in `OMKeyRenameRequestWithFSO` in `validateAndUpdateCache()` on [line 103](https://github.com/apache/ozone/blob/master/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyRenameRequestWithFSO.java#L103) we are checking again, if the `toKeyName` is empty or not.
for me it would be confusing to do the changes there, even though the checks for an empty `toKeyName` are not 100% necessary. this whole thing is not an issue in the case of a non FSO bucket is because we are doing a check in the `rename()` method in `BasicRootedOzoneFileSystem` (after the point we are returning with `renameFSO()`, where I did the changes in my current patch), check [here](https://github.com/apache/ozone/blob/master/hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicRootedOzoneFileSystem.java#L390L399)
```
} else if (dstStatus.isDirectory()) {
// If dst is a directory, rename source as subpath of it.
// for example rename /source to /dst will lead to /dst/source
dst = new Path(dst, src.getName());
FileStatus[] statuses;
try {
statuses = listStatus(dst);
} catch (FileNotFoundException fnde) {
statuses = null;
}
```
that is why I think the handling should happen here in `BasicRootedOzoneFileSystem` this way. let me know if I misunderstood something and what you think of this approach.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] dombizita commented on a diff in pull request #4478: HDDS-8242. Rename operation not working with FSO bucket destination
Posted by "dombizita (via GitHub)" <gi...@apache.org>.
dombizita commented on code in PR #4478:
URL: https://github.com/apache/ozone/pull/4478#discussion_r1152070128
##########
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/OFSPath.java:
##########
@@ -163,6 +163,14 @@ public String getKeyName() {
return keyName;
}
+ /**
+ * Returns the last element of the KeyName, which can either be a file name
+ * or the leaf directory/bucket.
Review Comment:
ohh, thanks @jojochuang, you are right, it can't be a bucket :) I updated the comment accordingly
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] dombizita commented on a diff in pull request #4478: HDDS-8242. Rename operation not working with FSO bucket destination
Posted by "dombizita (via GitHub)" <gi...@apache.org>.
dombizita commented on code in PR #4478:
URL: https://github.com/apache/ozone/pull/4478#discussion_r1152073657
##########
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/OFSPath.java:
##########
@@ -163,6 +163,14 @@ public String getKeyName() {
return keyName;
}
+ /**
+ * Returns the last element of the KeyName, which can either be a file name
+ * or the leaf directory/bucket.
+ */
+ public String getFileName() {
Review Comment:
yes, you are right, that's why I documented that in the javadoc. let me know if you have a better name idea for the method :)
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] kerneltime commented on pull request #4478: HDDS-8242. Rename operation not working with FSO bucket destination
Posted by "kerneltime (via GitHub)" <gi...@apache.org>.
kerneltime commented on PR #4478:
URL: https://github.com/apache/ozone/pull/4478#issuecomment-1485415508
cc @sumitagrawl
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sadanand48 commented on a diff in pull request #4478: HDDS-8242. Rename operation not working with FSO bucket destination
Posted by "sadanand48 (via GitHub)" <gi...@apache.org>.
sadanand48 commented on code in PR #4478:
URL: https://github.com/apache/ozone/pull/4478#discussion_r1152833154
##########
hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicRootedOzoneFileSystem.java:
##########
@@ -430,8 +430,14 @@ private boolean renameFSO(OzoneBucket bucket,
// construct src and dst key paths
String srcKeyPath = srcPath.getNonKeyPathNoPrefixDelim() +
OZONE_URI_DELIMITER + srcPath.getKeyName();
- String dstKeyPath = dstPath.getNonKeyPathNoPrefixDelim() +
- OZONE_URI_DELIMITER + dstPath.getKeyName();
+ String dstKeyPath;
+ if (dstPath.isBucket()) {
+ dstKeyPath = dstPath.getNonKeyPathNoPrefixDelim() +
+ OZONE_URI_DELIMITER + srcPath.getFileName();
+ } else {
+ dstKeyPath = dstPath.getNonKeyPathNoPrefixDelim() +
+ OZONE_URI_DELIMITER + dstPath.getKeyName();
Review Comment:
> For OMKeyRenamRequest, we should not allow empty key name, that part is ok. Only for OMKeyRenameRequestWithFSO, can remove toKeyName.length() == 0 check
preExecute behaviour is common for both FSO and non-FSO request types. In this case we would need to add a separate preExecute method for FSO to overcome this check. Also for Non-FSO renames it is the client who handles this part to reset the `toKeyName` if empty. I think it is better to keep the behaviour consistent across bucket types . OM should only care about renaming a valid source to a valid dest key and not worry about the paths, This is just my opinion.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai merged pull request #4478: HDDS-8242. Rename operation not working with FSO bucket destination
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai merged PR #4478:
URL: https://github.com/apache/ozone/pull/4478
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org