-
Notifications
You must be signed in to change notification settings - Fork 588
HDDS-4514. AllocateBlock : lookup and update open file table for the given path #1679
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please address below minor comment and fix failed unit test, : ).
...manager/src/main/java/org/apache/hadoop/ozone/om/response/key/OMAllocateBlockResponseV1.java
Outdated
Show resolved
Hide resolved
TestOzoneFileSystemV1#testListStatusOnLargeDirectory is failing in the build, interestingly it is passing consistently in my local env. Added log messages to debug it. Will monitor the results in next build. |
|
@linyiqun I've addressed your comments. Could you please review it again when you get a chance. Thanks! |
linyiqun
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1.
Thanks for addressing the comments, @rakeshadr .
...e-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMAllocateBlockRequestV1.java
Show resolved
Hide resolved
...e-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMAllocateBlockRequestV1.java
Show resolved
Hide resolved
bharatviswa504
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM, few comments.
...ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/file/OMFileRequest.java
Show resolved
Hide resolved
| private static void checkDirectoryAlreadyExists(String keyName, | ||
| OzoneManager ozoneManager, boolean reachedLastPathComponent) | ||
| throws IOException { | ||
| // Reached last component, which would be a file. Returns its parentID. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: Comment says Returns its parentID.
In code we just check if lastComponent is true and fileSystem enabled, we return exception.
And also as current V1 always assumes fs enabled true, do we need && ozoneManager.getEnableFileSystemPaths() check?
And also these 3 lines can be in the calling method, as it is not a utility method which is called by different classes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, Done the changes!
| omClientResponse = new OMAllocateBlockResponse(omResponse.build(), | ||
| openKeyInfo, clientID, omVolumeArgs, omBucketInfo.copyObject()); | ||
|
|
||
| omClientResponse = getOmClientResponse(clientID, omResponse, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't update omVolumeArgs, but still reading and passing to Response, which is not required.
Not related to your patch BTW, we can fix this in master and get to branch.
...ger/src/test/java/org/apache/hadoop/ozone/om/response/key/TestOMAllocateBlockResponseV1.java
Outdated
Show resolved
Hide resolved
bharatviswa504
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 LGTM
...one-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMAllocateBlockRequest.java
Show resolved
Hide resolved
|
Thank you @linyiqun , @bharatviswa504 for the useful review comments. Merging it to the feature branch. |
What changes were proposed in this pull request?
This task is to use open file table for the allocate block operations. This has been identified as part of Hive TPCDS benchmark test
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-4514
How was this patch tested?
Added UT cases. Also, verified by running hive tpcds test