You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by GitBox <gi...@apache.org> on 2020/12/10 08:02:07 UTC
[GitHub] [ozone] ayushtkn opened a new pull request #1682: HDDS-4574. Inconsistencies in case the parent of a open file is deleted/renamed.
ayushtkn opened a new pull request #1682:
URL: https://github.com/apache/ozone/pull/1682
## What changes were proposed in this pull request?
Check whether parent exist before commit key to counter inconsistencies due to it. Scenario mentioned in the jira.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-4574
## How was this patch tested?
Added UT.
----------------------------------------------------------------
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.
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] ayushtkn commented on pull request #1682: HDDS-4574. Inconsistencies in case the parent of a open file is deleted/renamed.
Posted by GitBox <gi...@apache.org>.
ayushtkn commented on pull request #1682:
URL: https://github.com/apache/ozone/pull/1682#issuecomment-745260127
Thanx @mukul1987 I have rebased the PR
----------------------------------------------------------------
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.
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] ayushtkn commented on a change in pull request #1682: HDDS-4574. Inconsistencies in case the parent of a open file is deleted/renamed.
Posted by GitBox <gi...@apache.org>.
ayushtkn commented on a change in pull request #1682:
URL: https://github.com/apache/ozone/pull/1682#discussion_r544302585
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMAllocateBlockRequest.java
##########
@@ -192,6 +193,17 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
KEY_NOT_FOUND);
}
+ if (ozoneManager.getEnableFileSystemPaths()) {
+ // Ensure the parent exist.
+ if (!"".equals(OzoneFSUtils.getParent(keyName))
Review comment:
Thanx Rakesh for sharing the details, makes sense, Will explore more :-)
I have removed the check from `AllocateBlock` as suggested.
@bharatviswa504 Please give a check once agin, if things make sense now. Thanx...
----------------------------------------------------------------
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.
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] mukul1987 commented on pull request #1682: HDDS-4574. Inconsistencies in case the parent of a open file is deleted/renamed.
Posted by GitBox <gi...@apache.org>.
mukul1987 commented on pull request #1682:
URL: https://github.com/apache/ozone/pull/1682#issuecomment-745201603
@ayushtkn can you please rebase the patch ?
----------------------------------------------------------------
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.
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] rakeshadr commented on a change in pull request #1682: HDDS-4574. Inconsistencies in case the parent of a open file is deleted/renamed.
Posted by GitBox <gi...@apache.org>.
rakeshadr commented on a change in pull request #1682:
URL: https://github.com/apache/ozone/pull/1682#discussion_r540047078
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequest.java
##########
@@ -172,6 +173,16 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
throw new OMException("Failed to commit key, as " + dbOpenKey +
"entry is not found in the OpenKey table", KEY_NOT_FOUND);
}
+
+ // Ensure the parent exist.
+ if (!"".equals(OzoneFSUtils.getParent(keyName))
Review comment:
Can you move this check inside `ozoneManager.getEnableFileSystemPaths() == true` case to do this only for FileSystem api.
For example, bucket#createKey & OzoneOutputStream#close() in turn make OMKeyCommitRequest call?
----------------------------------------------------------------
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.
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] rakeshadr commented on a change in pull request #1682: HDDS-4574. Inconsistencies in case the parent of a open file is deleted/renamed.
Posted by GitBox <gi...@apache.org>.
rakeshadr commented on a change in pull request #1682:
URL: https://github.com/apache/ozone/pull/1682#discussion_r540047078
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequest.java
##########
@@ -172,6 +173,16 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
throw new OMException("Failed to commit key, as " + dbOpenKey +
"entry is not found in the OpenKey table", KEY_NOT_FOUND);
}
+
+ // Ensure the parent exist.
+ if (!"".equals(OzoneFSUtils.getParent(keyName))
Review comment:
Can you move this check inside `ozoneManager.getEnableFileSystemPaths() == true` case to do this only for FileSystem api.
For example, bucket#createKey & OzoneOutputStream#close() in turn make OMKeyCommitRequest call.
----------------------------------------------------------------
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.
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] ayushtkn commented on a change in pull request #1682: HDDS-4574. Inconsistencies in case the parent of a open file is deleted/renamed.
Posted by GitBox <gi...@apache.org>.
ayushtkn commented on a change in pull request #1682:
URL: https://github.com/apache/ozone/pull/1682#discussion_r543861121
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMAllocateBlockRequest.java
##########
@@ -192,6 +193,17 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
KEY_NOT_FOUND);
}
+ if (ozoneManager.getEnableFileSystemPaths()) {
+ // Ensure the parent exist.
+ if (!"".equals(OzoneFSUtils.getParent(keyName))
Review comment:
Thanx @bharatviswa504 for the review.
>Right now delete/rename is not an atomic operation, so will just checking the parent Dir is enough?
Is there anything else, that can be checked? Yahh, there might be some issue because of non atomic behaviour delete/rename. Will pulling this check inside lock, solve them or lead to a better state?
>And also for performance reasons, can we leave allocateBlock, as during KeyCommit anyway we shall figure it out?
Rakesh suggested to add the check in `allocateBlock`, I think for a `fail-fast` approach, If we tend to fail during the commit phase and we know that at the `allocateBlock` phase, we should fail at an earlier stage itself, rather than going till the end. Might save a messed up client, if he is writing a big file.
But I am not very aware about the performance impacts and HDDS-2939 both, so I am ok both ways.
@rakeshadr any thoughts on 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.
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] bharatviswa504 commented on a change in pull request #1682: HDDS-4574. Inconsistencies in case the parent of a open file is deleted/renamed.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #1682:
URL: https://github.com/apache/ozone/pull/1682#discussion_r543570014
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMAllocateBlockRequest.java
##########
@@ -192,6 +193,17 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
KEY_NOT_FOUND);
}
+ if (ozoneManager.getEnableFileSystemPaths()) {
+ // Ensure the parent exist.
+ if (!"".equals(OzoneFSUtils.getParent(keyName))
Review comment:
And also for performance reasons, can we leave allocateBlock, as during KeyCommit anyway we shall figure it out?
As this will save one extra DB op, (this might not be a problem again once HDDS-2939 comes in, as it has directory cache)
But I am fine with if we want to catch early also? Just thought of bringing this here.
----------------------------------------------------------------
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.
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] bharatviswa504 commented on a change in pull request #1682: HDDS-4574. Inconsistencies in case the parent of a open file is deleted/renamed.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #1682:
URL: https://github.com/apache/ozone/pull/1682#discussion_r545609085
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMAllocateBlockRequest.java
##########
@@ -192,6 +193,17 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
KEY_NOT_FOUND);
}
+ if (ozoneManager.getEnableFileSystemPaths()) {
+ // Ensure the parent exist.
+ if (!"".equals(OzoneFSUtils.getParent(keyName))
Review comment:
>Is there anything else, that can be checked? Yahh, there might be some issue because of non atomic behaviour delete/rename. Will pulling this check inside lock, solve them or lead to a better state?
As due to non-atomic behavior whole path check can help here. But this issue will be solved once HDDS-2939 comes in. And also this check is done under lock even right now. Let's have this way for now.
----------------------------------------------------------------
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.
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] bharatviswa504 commented on pull request #1682: HDDS-4574. Inconsistencies in case the parent of a open file is deleted/renamed.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on pull request #1682:
URL: https://github.com/apache/ozone/pull/1682#issuecomment-747909254
Thank You @ayushtkn for the contribution and @rakeshadr 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.
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] rakeshadr commented on a change in pull request #1682: HDDS-4574. Inconsistencies in case the parent of a open file is deleted/renamed.
Posted by GitBox <gi...@apache.org>.
rakeshadr commented on a change in pull request #1682:
URL: https://github.com/apache/ozone/pull/1682#discussion_r544123278
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMAllocateBlockRequest.java
##########
@@ -192,6 +193,17 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
KEY_NOT_FOUND);
}
+ if (ozoneManager.getEnableFileSystemPaths()) {
+ // Ensure the parent exist.
+ if (!"".equals(OzoneFSUtils.getParent(keyName))
Review comment:
>And also for performance reasons, can we leave allocateBlock, as during KeyCommit anyway we shall figure it out?
On a second thought, it makes sense to me to avoid any additional DB calls which can affect the OM performance. Most of the cases, parent directories would exists in RocksDB offheap cache even then I would agree to skip parentDir check considering the cost of DB call if not exist.
On the other side, I think there won't be any correctness issue if we skip parent dir check in AllocateBlockReq. Later, we can bring this` fast-fail` check based on user demand. Considering that, I'm +1 to skip extra DB check in AllocateBlockReq now.
Thanks a lot @ayushtkn for your patience and taking care this. I agree to revert the changes from OmAllocateBlockRequest.
>But I am not very aware about the performance impacts and HDDS-2939 both
In HDDS-2939, in order to fetch openFileName, it requires to traverse top-down model and parentDir check won't add any additional overhead. For example, say open file /a/b/c/d/file1.
_HDDS-2939 branch_: OM will fetch /a, /a/b, /a/b/c, /a/b/c/d from DirTable and finally get /a/b/c/d/file1 from OpenFileTable. Here, one idea is to build DirCache to reduce DB overhead. @bharatviswa504 was talking about this part in his previous comment.
_Master branch_: OM will do point look up by fetching /a/b/c/d from KeyTable. Here, existing RocksDB offheap cache would help.
----------------------------------------------------------------
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.
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] bharatviswa504 commented on a change in pull request #1682: HDDS-4574. Inconsistencies in case the parent of a open file is deleted/renamed.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #1682:
URL: https://github.com/apache/ozone/pull/1682#discussion_r543567242
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMAllocateBlockRequest.java
##########
@@ -192,6 +193,17 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
KEY_NOT_FOUND);
}
+ if (ozoneManager.getEnableFileSystemPaths()) {
+ // Ensure the parent exist.
+ if (!"".equals(OzoneFSUtils.getParent(keyName))
Review comment:
Just a question: Right now delete/rename is not an atomic operation, so will just checking the parent Dir is enough?
But this will help, once we have HDDS-2939 where we have atomic delete/renames.
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMAllocateBlockRequest.java
##########
@@ -192,6 +193,17 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
KEY_NOT_FOUND);
}
+ if (ozoneManager.getEnableFileSystemPaths()) {
+ // Ensure the parent exist.
+ if (!"".equals(OzoneFSUtils.getParent(keyName))
Review comment:
And also for performance reasons, can we leave allocateBlock, as during KeyCommit anyway we shall figure it out?
As this will save one extra DB op, (this might not be a problem again once HDDS-2939 comes in, as it has directory cache)
----------------------------------------------------------------
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.
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] ayushtkn commented on a change in pull request #1682: HDDS-4574. Inconsistencies in case the parent of a open file is deleted/renamed.
Posted by GitBox <gi...@apache.org>.
ayushtkn commented on a change in pull request #1682:
URL: https://github.com/apache/ozone/pull/1682#discussion_r540206764
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequest.java
##########
@@ -172,6 +173,16 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
throw new OMException("Failed to commit key, as " + dbOpenKey +
"entry is not found in the OpenKey table", KEY_NOT_FOUND);
}
+
+ // Ensure the parent exist.
+ if (!"".equals(OzoneFSUtils.getParent(keyName))
Review comment:
Thanx @rakeshadr for the review.
I have moved the check inside `ozoneManager.getEnableFileSystemPaths() == true` and added the same logic and tests for `OMAllocateBlockRequest` as well. Please have a look..
----------------------------------------------------------------
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.
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] bharatviswa504 merged pull request #1682: HDDS-4574. Inconsistencies in case the parent of a open file is deleted/renamed.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 merged pull request #1682:
URL: https://github.com/apache/ozone/pull/1682
----------------------------------------------------------------
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.
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] rakeshadr commented on a change in pull request #1682: HDDS-4574. Inconsistencies in case the parent of a open file is deleted/renamed.
Posted by GitBox <gi...@apache.org>.
rakeshadr commented on a change in pull request #1682:
URL: https://github.com/apache/ozone/pull/1682#discussion_r540047078
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequest.java
##########
@@ -172,6 +173,16 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
throw new OMException("Failed to commit key, as " + dbOpenKey +
"entry is not found in the OpenKey table", KEY_NOT_FOUND);
}
+
+ // Ensure the parent exist.
+ if (!"".equals(OzoneFSUtils.getParent(keyName))
Review comment:
Can you move this check inside `ozoneManager.getEnableFileSystemPaths() == true` case to do this only for FileSystem api.
Should we need this check for bucket#createKey & OzoneOutputStream#close() in turn make OMKeyCommitRequest call?
----------------------------------------------------------------
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.
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] ayushtkn commented on a change in pull request #1682: HDDS-4574. Inconsistencies in case the parent of a open file is deleted/renamed.
Posted by GitBox <gi...@apache.org>.
ayushtkn commented on a change in pull request #1682:
URL: https://github.com/apache/ozone/pull/1682#discussion_r543861121
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMAllocateBlockRequest.java
##########
@@ -192,6 +193,17 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
KEY_NOT_FOUND);
}
+ if (ozoneManager.getEnableFileSystemPaths()) {
+ // Ensure the parent exist.
+ if (!"".equals(OzoneFSUtils.getParent(keyName))
Review comment:
Thanx @bharatviswa504 for the review.
Rakesh suggested to add the check in `allocateBlock`, I think for a `fail-fast` approach, If we tend to fail during the commit phase and we know that at the `allocateBlock` phase, we should fail at an earlier stage itself, rather than going till the end. Might save a messed up client, if he is writing a big file.
But I am not very aware about the performance impacts and HDDS-2939 both, so I am ok both ways.
@rakeshadr any thoughts on 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.
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