-
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-28042 Snapshot corruptions due to non-atomic rename within same filesystem #5369
Conversation
FYI @ujjawal4046 |
🎊 +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. |
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.
Thanks @ujjawal4046 for looking into this.
+1 non binding.
|
@mymeiyi could you please take a look at For instance, for test
|
Similarly, on |
🎊 +1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
@@ -158,6 +158,7 @@ public void testGrantGlobal1() throws Exception { | |||
|
|||
TestHDFSAclHelper.createTableAndPut(TEST_UTIL, table); | |||
snapshotAndWait(snapshot1, table); | |||
snapshotAndWait(snapshot2, 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.
Does this test really need this change, or is this part of the PR by mistake?
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.
it actually does, i am still trying to understand why the order of granting permissions to global user matter in the order. e.g. before table creation, granting global user permission allows user to scan snapshot but after table creation if we grant it, it doesn't work.
perhaps some bug in coproc SnapshotScannerHDFSAclController
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.
Ack!
// check scheme, e.g. file, hdfs | ||
if ( | ||
workingURI.getScheme() == null | ||
&& (rootURI.getScheme() != null && !rootURI.getScheme().equalsIgnoreCase("file")) |
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.
Why we need the extra check for the "file" URI? Wouldn't it be enough to return "true" here if workingURI scheme is null and rootURI is not?
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.
with "file", we could have ignored local fs, but you are right that we don't need such explicit check, let me make this change, thanks
the ordering issue with |
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, +1.
Just as an extra note, "fs.rename" may not always be atomic (think of some object store file systems), and the ultimate solution for this problem would be something similar to what SFT did for the store files rename, but that could be worked on a different jira.
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
agree to using SFT 👍 |
🎊 +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. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
Thanks for the reviews @wchevreuil @mnpoonia @ujjawal4046 @Abhey !! |
… filesystem (#5369) Co-authored-by: Ujjawal <[email protected]> Signed-off-by: Wellington Chevreuil <[email protected]> Signed-off-by: Abhey Rana <[email protected]> Signed-off-by: Ujjawal <[email protected]> Signed-off-by: Aman Poonia <[email protected]>
… filesystem (#5372) (#5369) Co-Authored-By: Ujjawal <[email protected]> Signed-off-by: Wellington Chevreuil <[email protected]> Signed-off-by: Abhey Rana <[email protected]> Signed-off-by: Ujjawal <[email protected]> Signed-off-by: Aman Poonia <[email protected]>
… filesystem (#5372) (#5369) Co-Authored-By: ujjawal4046 <[email protected]> Signed-off-by: Wellington Chevreuil <[email protected]> Signed-off-by: Abhey Rana <[email protected]> Signed-off-by: Ujjawal <[email protected]> Signed-off-by: Aman Poonia <[email protected]>
… filesystem (#5372) (#5369) Co-Authored-By: ujjawal4046 <[email protected]> Signed-off-by: Wellington Chevreuil <[email protected]> Signed-off-by: Abhey Rana <[email protected]> Signed-off-by: Ujjawal <[email protected]> Signed-off-by: Aman Poonia <[email protected]>
… filesystem (apache#5369) Co-authored-by: Ujjawal <[email protected]> Signed-off-by: Wellington Chevreuil <[email protected]> Signed-off-by: Abhey Rana <[email protected]> Signed-off-by: Ujjawal <[email protected]> Signed-off-by: Aman Poonia <[email protected]>
… filesystem (apache#5372) (apache#5369) Co-Authored-By: ujjawal4046 <[email protected]> Signed-off-by: Wellington Chevreuil <[email protected]> Signed-off-by: Abhey Rana <[email protected]> Signed-off-by: Ujjawal <[email protected]> Signed-off-by: Aman Poonia <[email protected]> (cherry picked from commit 61250ad) Change-Id: I8f93a9b89df3c1195d427ac1b67001708e716b49
Co-authored-by: Ujjawal <[email protected]>
Jira: HBASE-28042