-
Notifications
You must be signed in to change notification settings - Fork 9.1k
HDDS-1371. Download RocksDB checkpoint from OM Leader to Follower. #703
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
|
Posted initial patch. Will add unit tests soon. |
This comment has been minimized.
This comment has been minimized.
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 should add unit tests.
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 should probably use Paths.get to join the path here. Using / breaks Windows. Even though we don't officially support Windows we should make the path logic platform agnostic.
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.
What happens during upgrade if two OMs are using different compression schemes? Perhaps we should file a todo to make the compression scheme configurable later.
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.
Nitpick: Need a space after //.
Also in other places in the same patch.
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.
Let's log the dir name here.
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.
Is there any existing code in Apache commons/HDFS which does this? I am sure there must be. One thing we are potentially missing here is file attributes.
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.
I didn't understand this. Could you explain a bit what it does?
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.
Could you add a Javadoc for the class please?
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.
Should use getHttpPolicy here.
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/OmUtils.java
Outdated
Show resolved
Hide resolved
1b0ea0c to
5d0d70f
Compare
|
Thank you @arp7 for the review. Addressed your review comments and added a unit test. |
This comment has been minimized.
This comment has been minimized.
4d8bf5d to
920852e
Compare
This comment has been minimized.
This comment has been minimized.
|
/retest |
This comment has been minimized.
This comment has been minimized.
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/HddsUtils.java
Outdated
Show resolved
Hide resolved
hadoop-hdds/common/src/main/java/org/apache/hadoop/utils/db/DBCheckpoint.java
Outdated
Show resolved
Hide resolved
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/OmUtils.java
Outdated
Show resolved
Hide resolved
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java
Outdated
Show resolved
Hide resolved
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.
Thank You @hanishakoneru for the contribution.
Overall patch LGTM. I have a few minor comments.
|
/retest |
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java
Outdated
Show resolved
Hide resolved
|
💔 -1 overall
This message was automatically generated. |
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, pending CI. Take care of two minor comments during commit.
As discussed offline using of the new property ozone.http.policy for ozone, can we open a new Jira for that?
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/HddsUtils.java
Outdated
Show resolved
Hide resolved
hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConfigKeys.java
Outdated
Show resolved
Hide resolved
|
/retest |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Fixed related unit test failures. |
|
/retest |
This comment has been minimized.
This comment has been minimized.
|
/retest |
This comment has been minimized.
This comment has been minimized.
|
💔 -1 overall
This message was automatically generated. |
|
Thank You @hanishakoneru for the contribution. |
Internally at Linkedin we've observed that excluding specific `js` and `css` through rat gradle-plugin causes the ligradle build to not run the unit-tests. To fix it, this patch removes those files from rat excludes and adds the samza license to the individual files. Author: Shanthoosh Venkataraman <[email protected]> Reviewers: Prateek Maheshwari <[email protected]> Closes apache#703 from shanthoosh/master
No description provided.