-
Notifications
You must be signed in to change notification settings - Fork 3.4k
HBASE-25475: Improve UT added as part of HBASE-25445 in TestSplitWALManager #2855
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
dasanjan1296
commented
Jan 7, 2021
- This is to address post-merge feedback from HBASE-25445: Use WAL FS instead of master FS in SplitWALManager #2844
|
@wchevreuil Thanks for the feedback on #2844. Please take a look. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
| testUtil2.waitFor(30000, () -> executor.getProcedures().stream() | ||
| .filter(p -> p instanceof SplitTableRegionProcedure) | ||
| .map(p -> (SplitTableRegionProcedure) p) | ||
| .anyMatch(p -> TABLE_NAME.equals(p.getTableName())) && splitProcedure.isSuccess()); | ||
| testUtil2.getMiniHBaseCluster().killRegionServer( | ||
| testUtil2.getMiniHBaseCluster().getRegionServer(0).getServerName()); | ||
| testUtil2.getMiniHBaseCluster().startRegionServer(); | ||
| testUtil2.waitUntilNoRegionsInTransition(); |
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.
Test looks fine, functionally wise, but could we simplify it and make it more clear by directly testing the log splitting? Sorry, don't want to be picky, it just looks we are doing more than needed here, maybe it would be simpler to just to do same thing as testSplitLogs(), after making sure wal is under a different file system. You could move current testSplitLogs() to a separate method that would be called by both this test and testSplitLogs.
WDYT, @dasanjan1296 ?
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.
Yes, I agree with you. Simply doing a split wal operation should be enough to test this particular scenario. I went overboard with the test while trying to explore a few things. I've updated the test per your suggestion. PTAL.
|
💔 -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. |
…anager (#2855) Signed-off-by: Wellington Chevreuil <[email protected]>
…anager (#2855) Signed-off-by: Wellington Chevreuil <[email protected]>
…anager (#2855) Signed-off-by: Wellington Chevreuil <[email protected]>
…anager (#2855) Signed-off-by: Wellington Chevreuil <[email protected]>
…anager (apache#2855) Signed-off-by: Wellington Chevreuil <[email protected]> (cherry picked from commit fcf255c) Change-Id: Ie6da98374bc029efad1aa89391f9ef15011dd4c6