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: [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]
