-
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-25445: Use WAL FS instead of master FS in SplitWALManager #2844
Conversation
dasanjan1296
commented
Jan 5, 2021
- If 'hbase.wal.dir' and 'hbase.rootdir' are configured to different filesystem instances, SplitWALRemoteProcedure archived split WAL failed since SplitWALManager using wrong fs instance. SplitWALManager should use WAL corresponding fs instance.
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
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.
Any test case to capture this regression? Else LGTM.
@ramkrish86 I have added some testing details in the corresponding JIRA for reproducing the error. |
💔 -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.
+1, pending QA
🎊 +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.
Please add a UT at TestSplitWALManager for this condition, as already suggested by @ramkrish86 . This is a simple, yet an important fix, we should safeguard SplitWALManager from inadvertent changes that could suppress this fix.
🎊 +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.
Nice test, just a couple of nits
hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestSplitWALManager.java
Outdated
Show resolved
Hide resolved
hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestSplitWALManager.java
Outdated
Show resolved
Hide resolved
hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestSplitWALManager.java
Outdated
Show resolved
Hide resolved
…me local variables
73e53f7
to
59ca6d1
Compare
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, Thanks @dasanjan1296
Once QA results are in, will merge this PR. |
I think there seems some problem with Jenkins, the recent build is aborted and I am not able to schedule a new build. |
New build is triggered |
🎊 +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. |
Build is taking more time. Let me merge this PR, I have validated test locally and we also have result available here https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2844/5/testReport/org.apache.hadoop.hbase.master/TestSplitWALManager/ |
Signed-off-by: Pankaj <pankajkumar@apache.org> Signed-off-by: ramkrish86 <ramkrishna@apache.org> Signed-off-by: Viraj Jasani <vjasani@apache.org>
Signed-off-by: Pankaj <pankajkumar@apache.org> Signed-off-by: ramkrish86 <ramkrishna@apache.org> Signed-off-by: Viraj Jasani <vjasani@apache.org>
Signed-off-by: Pankaj <pankajkumar@apache.org> Signed-off-by: ramkrish86 <ramkrishna@apache.org> Signed-off-by: Viraj Jasani <vjasani@apache.org>
Signed-off-by: Pankaj <pankajkumar@apache.org> Signed-off-by: ramkrish86 <ramkrishna@apache.org> Signed-off-by: Viraj Jasani <vjasani@apache.org>
🎊 +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.
I had some nits regarding the current UT, but since this got merged already, we may follow on another jira.
@@ -86,6 +98,58 @@ public void teardown() throws Exception { | |||
TEST_UTIL.shutdownMiniCluster(); | |||
} | |||
|
|||
@Test | |||
public void testWALArchiveWithDifferentWalAndRootFS() throws Exception{ | |||
HBaseTestingUtility test_util_2 = new HBaseTestingUtility(); |
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.
nit: non static variables should follow camel case style.
} | ||
executor.submitProcedure(splitProcedure); | ||
LOG.info("Submitted SplitProcedure."); | ||
test_util_2.waitFor(30000, () -> executor.getProcedures().stream() |
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.
Shouldn't we filter by the split procedure submitted and assert it has completed success?