-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
HBASE-28216 HDFS erasure coding support for table data dirs #5579
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
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.
Looks good. One question: will hbase shell show erasure coding policy of a table too?
Great question! That actually slipped my mind and I will get that working next week |
This comment was marked as off-topic.
This comment was marked as off-topic.
hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/CreateTableProcedure.java
Show resolved
Hide resolved
hbase-server/src/main/java/org/apache/hadoop/hbase/fs/ErasureCodingUtils.java
Show resolved
Hide resolved
hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/ModifyTableProcedure.java
Outdated
Show resolved
Hide resolved
hbase-client/src/main/java/org/apache/hadoop/hbase/client/TableDescriptorBuilder.java
Show resolved
Hide resolved
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/CompactedHFilesDischarger.java
Show resolved
Hide resolved
|
||
@BeforeClass | ||
public static void beforeClass() throws Exception { | ||
UTIL.startMiniDFSCluster(6); // 6 necessary for RS-6-3-1024k |
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.
Comment not related to test:
Just wondering, what happens if some one mistakenly sets tis policy on a cluster with less than 6 nodes? Can the table/system be recovered/fixed?
Also, do we have any checks to ensure ECP cannot be configured if this condition is not satisfied? Is it worth adding?
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.
Great question. Typically a hdfs client will fail to write in this case (it would actually be better to have at least 9 nodes as 6 nodes provide no redundancy). In the case of HBase, it looks like compaction will fail, in which case administrator will need to step in and update the EC policy of the table.
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.
To check if the EC policy is compliant with rack/host setup, check out these two jiras:
https://issues.apache.org/jira/browse/HDFS-14061
https://issues.apache.org/jira/browse/HDFS-12946
It's going to be a sizeable change so I'd suggest to leave that out of this PR.
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.
Oh ok, I didn't see those. For now I implemented a check in TableDescriptorChecker which sets the requested policy on a temp dir, and then tries to write to the temp dir. As you said, the write will fail. I have a test to validate that as well.
Will look at those 2 jiras for a follow-up improvement.
Hi @bbeaudreault thanks for the PR, overall looks good. Also have posted a few questions/reviews. Please let me know of your opinion on same. |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
…or per PR feedback
hbase-server/src/main/java/org/apache/hadoop/hbase/util/TableDescriptorChecker.java
Show resolved
Hide resolved
Thanks for the review @NihalJain! I believe I covered all of your feedback. I also made sure it works in the shell @jojochuang. |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
...-server/src/main/java/org/apache/hadoop/hbase/master/procedure/RestoreSnapshotProcedure.java
Show resolved
Hide resolved
...c/test/java/org/apache/hadoop/hbase/master/procedure/TestManageTableErasureCodingPolicy.java
Show resolved
Hide resolved
Hi @bbeaudreault added a new comment, please have a look. Otherwise changes looks good and all previous feedbacks seemed to be covered. Thanks :) |
0567f3a
to
4ec98a9
Compare
+1 to the change. Just realized, we may want to add sample command which sets EC in doc section of |
🎊 +1 overall
This message was automatically generated. |
If you don't mind, I might file a new jira for this. I took a look, and yes we put a bunch of examples in there but there doesn't seem to be any rhyme or reason. I wonder if we should improve the help text to list the possible options explicitly. We already hardcode the supported options in |
Sounds good. 🚀 |
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.
LGTM from my side. There are some comments that are worth addressing in follow jiras.
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
Thank you both for the reviews! |
Signed-off-by: Nihal Jain <nihaljain@apache.org> Signed-off-by: Wei-Chiu Chuang <weichiu@apache.org>
…ata dirs (apache#5579) Signed-off-by: Nihal Jain <nihaljain@apache.org> Signed-off-by: Wei-Chiu Chuang <weichiu@apache.org>
Since we require hadoop-3 for master and branch-3, I can use the EC APIs directly. If we want to backport to branch-2, we'll need to figure out how to do this with reflection.