You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "ashishkumar50 (via GitHub)" <gi...@apache.org> on 2023/09/07 08:02:37 UTC
[GitHub] [ozone] ashishkumar50 opened a new pull request, #5253: HDDS 9136. Throw exception when rename fails during moveToTrash
ashishkumar50 opened a new pull request, #5253:
URL: https://github.com/apache/ozone/pull/5253
## What changes were proposed in this pull request?
moveToTrash is not checking for return value of rename API. When rename fails due to various reasons, still moveToTrash assume it as success. This leads to inconsistent behaviour, one which is mentioned in Jira description(HDDS-9136).
In this PR, If there is failure in rename operation, we are throwing exception to know moveToTrash has failed.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-9136
## How was this patch tested?
Manually tested.
--
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] hemantk-12 commented on a diff in pull request #5253: HDDS-9136. Throw exception when rename fails during moveToTrash.
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #5253:
URL: https://github.com/apache/ozone/pull/5253#discussion_r1318961720
##########
hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicOzoneFileSystem.java:
##########
@@ -350,7 +350,19 @@ boolean processKey(List<String> keyList) throws IOException {
// TODO RenameKey needs to be changed to batch operation
for (String key : keyList) {
String newKeyName = dstKey.concat(key.substring(srcKey.length()));
- adapter.renameKey(key, newKeyName);
+ try {
+ adapter.renameKey(key, newKeyName);
+ } catch (OMException ome) {
+ LOG.error("rename key failed: {}. source:{}, destin:{}",
Review Comment:
nit: I think the log message can be better.
```
LOG.error("Key rename failed for source key: {} to " +
"destination key: {}.", key, newKeyName, ome);
```
or:
```
LOG.error("Key rename failed with error: {}. Source key: {}, " +
"Destination key: {}", ome.getMessage(), key, newKeyName);
```
Same for https://github.com/apache/ozone/pull/5253/files#diff-b2802792bb08b569714c90f10ecda42d917dd6329137188472010d2ea7476b1bR344
--
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] ChenSammi merged pull request #5253: HDDS-9136. Throw exception when rename fails during moveToTrash.
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi merged PR #5253:
URL: https://github.com/apache/ozone/pull/5253
--
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] ashishkumar50 commented on a diff in pull request #5253: HDDS-9136. Throw exception when rename fails during moveToTrash.
Posted by "ashishkumar50 (via GitHub)" <gi...@apache.org>.
ashishkumar50 commented on code in PR #5253:
URL: https://github.com/apache/ozone/pull/5253#discussion_r1319328378
##########
hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicOzoneFileSystem.java:
##########
@@ -350,7 +350,19 @@ boolean processKey(List<String> keyList) throws IOException {
// TODO RenameKey needs to be changed to batch operation
for (String key : keyList) {
String newKeyName = dstKey.concat(key.substring(srcKey.length()));
- adapter.renameKey(key, newKeyName);
+ try {
+ adapter.renameKey(key, newKeyName);
+ } catch (OMException ome) {
+ LOG.error("rename key failed: {}. source:{}, destin:{}",
Review Comment:
Done.
--
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] ashishkumar50 commented on a diff in pull request #5253: HDDS 9136. Throw exception when rename fails during moveToTrash
Posted by "ashishkumar50 (via GitHub)" <gi...@apache.org>.
ashishkumar50 commented on code in PR #5253:
URL: https://github.com/apache/ozone/pull/5253#discussion_r1318648601
##########
hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicRootedOzoneFileSystem.java:
##########
@@ -338,7 +338,19 @@ private class RenameIterator extends OzoneListingIterator {
boolean processKeyPath(List<String> keyPathList) throws IOException {
for (String keyPath : keyPathList) {
String newPath = dstPath.concat(keyPath.substring(srcPath.length()));
- adapterImpl.rename(this.bucket, keyPath, newPath);
+ try {
+ adapterImpl.rename(this.bucket, keyPath, newPath);
Review Comment:
Done
--
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] ashishkumar50 commented on pull request #5253: HDDS 9136. Throw exception when rename fails during moveToTrash
Posted by "ashishkumar50 (via GitHub)" <gi...@apache.org>.
ashishkumar50 commented on PR #5253:
URL: https://github.com/apache/ozone/pull/5253#issuecomment-1709819305
@sadanand48 @jojochuang Please help to review, thanks.
--
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 #5253: HDDS 9136. Throw exception when rename fails during moveToTrash
Posted by "sadanand48 (via GitHub)" <gi...@apache.org>.
sadanand48 commented on code in PR #5253:
URL: https://github.com/apache/ozone/pull/5253#discussion_r1318421121
##########
hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicRootedOzoneFileSystem.java:
##########
@@ -338,7 +338,19 @@ private class RenameIterator extends OzoneListingIterator {
boolean processKeyPath(List<String> keyPathList) throws IOException {
for (String keyPath : keyPathList) {
String newPath = dstPath.concat(keyPath.substring(srcPath.length()));
- adapterImpl.rename(this.bucket, keyPath, newPath);
+ try {
+ adapterImpl.rename(this.bucket, keyPath, newPath);
Review Comment:
Same change needs to be done in BasicOzoneFilesystem.
--
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] ashishkumar50 commented on pull request #5253: HDDS-9136. Throw exception when rename fails during moveToTrash.
Posted by "ashishkumar50 (via GitHub)" <gi...@apache.org>.
ashishkumar50 commented on PR #5253:
URL: https://github.com/apache/ozone/pull/5253#issuecomment-1713328363
> Overall changes look good to me.
>
> Is it possible to have some kind of unit or integration test for it? If yes, please add them.
I tried but it's difficult to have a test for this.
--
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