HBASE-23285 Sometimes walArchiveDir does not exist when separate the old WALs into different regionserver directories #819
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
wchevreuil
reviewed
Nov 14, 2019
Contributor
wchevreuil
left a comment
wchevreuil
left a comment
There was a problem hiding this comment.
Shouldn't the archive dir had already been created at WAL initialization here?
But,if there are no files, the regionserver directory will be cleared.
Do you mean, somewhere else, the RS specific wal dir gets deleted?
wchevreuil
reviewed
Nov 14, 2019
| while (v.length() < 1000) { | ||
| v.append(className); | ||
| } | ||
| this.value = Bytes.toBytes(v.toString()); |
Contributor
There was a problem hiding this comment.
Also, it seems we only use this value variable in the doPut method, so there's no need to make it a class attribute. Could be created at startAndWriteData and passed as parameter to doPut . If we still think it's worth keep it as global, then initialisation should be moved to setup method.
| @BeforeClass | ||
| public static void setUpBeforeClass() throws Exception { | ||
|
|
||
| Configuration conf = TEST_UTIL.getConfiguration(); |
Contributor
There was a problem hiding this comment.
As there's only one test method for now, TEST_UTIL could be made non-static and all initialisation logic moved to setUp method.
| TEST_UTIL.shutdownMiniCluster(); | ||
| } | ||
|
|
||
| protected void startAndWriteData() throws IOException, InterruptedException { |
Contributor
There was a problem hiding this comment.
Why protected modifier?
| try { | ||
| Thread.sleep(1000); | ||
| } catch (InterruptedException e) { | ||
| // continue |
Contributor
There was a problem hiding this comment.
Add logging to help troubleshooting eventual test failures?
| //start cleaner chore | ||
| Configuration conf = TEST_UTIL.getConfiguration(); | ||
| Path rootdir = CommonFSUtils.getWALRootDir(conf); | ||
| Path OLD_WALS_DIR = |
Contributor
There was a problem hiding this comment.
small nit: non-constant, variable names should follow camel case java conventions.
| Path rootdir = CommonFSUtils.getWALRootDir(conf); | ||
| Path OLD_WALS_DIR = | ||
| new Path(rootdir, HConstants.HREGION_OLDLOGDIR_NAME); | ||
| DirScanPool POOL = new DirScanPool(TEST_UTIL.getConfiguration()); |
Contributor
There was a problem hiding this comment.
small nit: non-constant, variable names should follow camel case java conventions.
| Thread thread = new Thread(() -> cleaner.chore()); | ||
| thread.setDaemon(true); | ||
| thread.start(); | ||
|
|
Contributor
There was a problem hiding this comment.
Shouldn't we wait for log cleaner thread to finish? This will likely give us false positives, as the test thread itself may finish before the log cleaner.
| thread.start(); | ||
|
|
||
| // Now roll the log | ||
| log.rollWriter(); |
Contributor
There was a problem hiding this comment.
We must assert what we are testing in this method. If we want to validate that wal file got deleted from the given RS oldWAL dir, then we should verify that the given folder is now empty.
| public TestSeparateOldWALCleaner() { | ||
| this.server = null; | ||
| this.tableName = null; | ||
|
|
Contributor
There was a problem hiding this comment.
These should be omitted, as object attributes are already null be default.
| StringBuilder v = new StringBuilder(className); | ||
| while (v.length() < 1000) { | ||
| v.append(className); | ||
| } |
Contributor
There was a problem hiding this comment.
Why such a loop to generate a value? Couldn't we simply create a byte[] here with the desired size?
Contributor
Author
|
|
💔 -1 overall
This message was automatically generated. |
…old WALs into different regionserver directories
|
💔 -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. |
Contributor
|
This PR causes the pre commit PR job NPE as the source repo is unreachable. Will close this one. Feel free to open a new one. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
HBASE-14247 Add a new config hbase.separate.oldlogdir.by.regionserver, the old wal dir will be separated by regionservers.
But,if there are no files, the regionserver directory will be cleared. CommonFSUtils.renameAndSetModifyTime(this.fs, p, newPath) will throw exception.