Uh oh!
There was an error while loading. Please reload this page.
HBASE-28064:Implement truncate_region command - #5462
Conversation
Apache-HBase
commented
Oct 16, 2023
💔 -1 overall
This message was automatically generated. |
0d2c228 to
3bc82c7CompareApache-HBase
commented
Oct 16, 2023
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Oct 16, 2023
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Oct 16, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Oct 16, 2023
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Oct 16, 2023
💔 -1 overall
This message was automatically generated. |
3bc82c7 to
2a6ae93CompareApache-HBase
commented
Oct 17, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Oct 17, 2023
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Oct 17, 2023
💔 -1 overall
This message was automatically generated. |
vaijosh
commented
Oct 17, 2023
recheck |
vaijosh
commented
Oct 17, 2023
retest |
vaijosh
commented
Oct 17, 2023
The failed tests is not looking related to my changes. Not sure if its a transient issue. |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| final MasterFileSystem mfs = env.getMasterServices().getMasterFileSystem(); | ||
| final Path tableDir = CommonFSUtils.getTableDir(mfs.getRootDir(), getTableName()); | ||
| final Configuration conf = env.getMasterConfiguration(); | ||
| HRegionFileSystem.deleteRegionFromFileSystem(conf, mfs.getFileSystem(), tableDir, region); |
There was a problem hiding this comment.
This would delete the region dir entirely. I don't see where we recreate the region dir, later. Isn't this a problem?
There was a problem hiding this comment.
This would delete the region dir entirely. I don't see where we recreate the region dir, later. Isn't this a problem?
While developing feature, I observed that subsequent task "TRUNCATE_REGION_MAKE_ONLINE" creates directory for region. So not added logic to create the directory explicitely.
There was a problem hiding this comment.
Thanks for pointing that. Yeah, that should be ok. Just one extra comment on the rollback: I see you are considering it only if we are in TRUNCATE_REGION_PRE_OPERATION. Couldn't we rollback from TRUNCATE_REGION_MAKE_OFFLINE too?
There was a problem hiding this comment.
Thanks for pointing that. Yeah, that should be ok. Just one extra comment on the rollback: I see you are considering it only if we are in TRUNCATE_REGION_PRE_OPERATION. Couldn't we rollback from TRUNCATE_REGION_MAKE_OFFLINE too?
Thanks @wchevreuil. This is valid concern. I this its already handled here
There was a problem hiding this comment.
Thanks for pointing it out!
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
wchevreuil
commented
Oct 17, 2023
Thanks for submitting this PR, looks like a great idea. I have a few questions/comments after an initial review. Please reply on those so that we can get this progressing. Thanks, Wellington. |
2a6ae93 to
523e4f9Comparevaijosh
commented
Oct 18, 2023
Thanks @wchevreuil for reviewing this. I have incorporated most of the review comments and answered the questions inline. Can you please check? |
Apache-HBase
commented
Oct 18, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Oct 18, 2023
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Oct 18, 2023
💔 -1 overall
This message was automatically generated. |
wchevreuil
left a comment
There was a problem hiding this comment.
LGTM, +1. The UT failures looks unrelated.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
NihalJain
commented
Oct 19, 2023
Thanks @vaijosh for working on this. Sorry for late review. I have added a few comments/suggestions. Please check. |
vaijosh
commented
Oct 20, 2023
|
523e4f9 to
3366b2aCompare…ctly from FS Implemented truncate_region command.
3366b2a to
07c871eCompareApache-HBase
commented
Oct 20, 2023
💔 -1 overall
This message was automatically generated. |
Uh oh!
There was an error while loading. Please reload this page.
NihalJain
commented
Oct 20, 2023
Skimmed latest push and updated comments. Looks fine high level. Also one suggestion, please avoid force push and keep just 1 commit. It's very hard to review what all changed. We anyways squash and merge, so should be fine to have several commits. |
Apache-HBase
commented
Oct 20, 2023
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Oct 20, 2023
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Oct 20, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Oct 20, 2023
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Oct 20, 2023
🎊 +1 overall
This message was automatically generated. |
vaijosh
commented
Oct 21, 2023
Thanks @NihalJain for the suggestion. But if this is not convenient, next time onwards I can do separate commits, and we can squash commits in the end. |
Yes it is a little inconvenient to compare across previous commits. And thanks for considering this. I have approved the PR. Looks good to me, please check once on users field comment before commit. |
Hi @NihalJain
Yes this was valid concern. But I have simulated and tested crash scenario and checked that we don't get NPE. |
NihalJain
commented
Oct 21, 2023
Awesome thanks for the due diligence and again thanks for working on this one. :) |
…ctly from FS Implemented review comment.
Apache-HBase
commented
Oct 21, 2023
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Oct 21, 2023
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Oct 21, 2023
🎊 +1 overall
This message was automatically generated. |
Signed-off-by: Wellington Chevreuil <wchevreuil@apache.org> Signed-off-by: Nihal Jain <nihaljain@apache.org>
Signed-off-by: Wellington Chevreuil <wchevreuil@apache.org> Signed-off-by: Nihal Jain <nihaljain@apache.org>
Signed-off-by: Wellington Chevreuil <wchevreuil@apache.org> Signed-off-by: Nihal Jain <nihaljain@apache.org>
Signed-off-by: Wellington Chevreuil <wchevreuil@apache.org> Signed-off-by: Nihal Jain <nihaljain@apache.org>
Signed-off-by: Wellington Chevreuil <wchevreuil@apache.org> Signed-off-by: Nihal Jain <nihaljain@apache.org>
HBASE-28064:Implement truncate_region command to truncate region directly from FS
Implemented truncate_region command and added unit/system tests.