HDDS-1551. Implement Bucket Write Requests to use Cache and DoubleBuf… #850
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bharatviswa504
force-pushed
the
HDDS-1551-1
branch
from
4418c56 to
efed9fc
Compare
|
💔 -1 overall
This message was automatically generated. |
bharatviswa504
force-pushed
the
HDDS-1551-1
branch
from
efed9fc to
b15e8d6
Compare
Contributor
Author
|
Fixed findbug and checkstyle issues. |
bharatviswa504
force-pushed
the
HDDS-1551-1
branch
from
b15e8d6 to
9483efb
Compare
|
💔 -1 overall
This message was automatically generated. |
bharatviswa504
force-pushed
the
HDDS-1551-1
branch
from
9483efb to
16b207a
Compare
|
💔 -1 overall
This message was automatically generated. |
bharatviswa504
force-pushed
the
HDDS-1551-1
branch
2 times, most recently
from
02a1a2d to
f763e1e
Compare
Contributor
Author
|
/retest |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
Contributor
hanishakoneru
left a comment
hanishakoneru
left a comment
There was a problem hiding this comment.
Thank you @bharatviswa504 for working on this.
I have a few comments, mostly NitPicks.
Contributor
There was a problem hiding this comment.
commitSetBucketPropertyInfoToDB() just calls commitCreateBucketInfoToDB() without any modification. We can directly call commitCreateBucketInfoToDB() here (and maybe rename it to commitBucketInfoToDB ? ).
Contributor
There was a problem hiding this comment.
How are these exceptions handled currently?
Contributor
Author
There was a problem hiding this comment.
Currently using ratisClient there is a TODO for RaftRetry failure exception, and I don't see anything being done for handling NotReplicatedException.
NotReplicatedException is thrown only for watch type requests. So, this exception will be never thrown in our HA case, as requests submitted through HA are Write requests.
RaftRetryFailure exception is thrown only from RatisClient, when using server this exception will not be thrown.
If you have any more comments, will update the TODO in next patch or I will take care of that in next jira if you are okay with it.
Contributor
There was a problem hiding this comment.
Should it not be Status.valueOf()? Or does this also give the same result?
Contributor
Author
There was a problem hiding this comment.
Here ordinal gives Position, and from that position finding the value from Status.values() (This return array of Status)
Contributor
There was a problem hiding this comment.
newCreateBucketInfo.setBucketInfo is done later. Should only set the creation time here.
Contributor
There was a problem hiding this comment.
Why do we need a volume lock here?
Contributor
Author
There was a problem hiding this comment.
Acquired volume lock here, so that deleteVolume cannot happen while creating bucket.
Take a scenario: volume created, then delete Volume and Create bucket in that volume came in parallel. If we don't acquire volume lock, there might be a chance to create a bucket in a deletd volume.
Contributor
There was a problem hiding this comment.
OMDummyCreateBucketResponse seems to be doing the same thing as OMBucketCreateResponse. Why do we need 2 different tests?
Contributor
Author
There was a problem hiding this comment.
This is added based on Arpit's comment in HDDS-1512. As we want to test OM Double Buffer Implementation without actual OM Responses too.
Contributor
There was a problem hiding this comment.
Typo: request is invalid
Contributor
There was a problem hiding this comment.
We can remove the prefix "OzoneManagerProtocolProtos" here (to avoid confusion)
Contributor
There was a problem hiding this comment.
bucketName is not used here.
Can we rename this method to something like addVolumeToDB?
Contributor
Author
|
Thank You @hanishakoneru for the review. |
Contributor
Author
|
/retest |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
Contributor
Author
|
/retest |
Contributor
Author
|
Last commit change is moved all the classes to package named bucket under request/response. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
bharatviswa504
force-pushed
the
HDDS-1551-1
branch
from
8d7bf41 to
7620007
Compare
Contributor
Author
|
/retest |
Contributor
Author
|
Rebased with trunk. |
bharatviswa504
force-pushed
the
HDDS-1551-1
branch
from
7620007 to
bf665af
Compare
|
💔 -1 overall
This message was automatically generated. |
Contributor
|
Thank you @bharatviswa504 . +1 pending CI. |
Contributor
Author
|
Thank You @hanishakoneru for the review. |
|
💔 -1 overall
This message was automatically generated. |
Contributor
Author
|
I have committed this to the trunk. |
shanthoosh
pushed a commit
to shanthoosh/hadoop
that referenced
this pull request
Oct 15, 2019
Author: Jagadish <[email protected]> Reviewers: Jagadish<[email protected]> Closes apache#850 from vjagadish1989/website-reorg37
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
…fer.