HBASE-26267 Don't try to recover WALs from a WAL dir which doesn't exist #3679
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. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
Apache9
reviewed
Sep 14, 2021
hbase-server/src/main/java/org/apache/hadoop/hbase/master/region/MasterRegion.java
Show resolved
Hide resolved
| create(); | ||
| } | ||
|
|
||
| public void create() throws IOException { |
There was a problem hiding this comment.
Better give it a more specific name? And does it need to be public? Protected is enough?
|
|
||
| // Re-create the MasterRegion and hit the MasterRegion#open() code-path | ||
| // (rather than bootstrap()) | ||
| super.create(); |
There was a problem hiding this comment.
This call is a bit confusing to me that, why there is a ‘super’. I think if we change create to a more specific name, we could just call the method directly, without adding the ‘super’ here?
|
🎊 +1 overall
This message was automatically generated. |
|
Thanks for the review, Duo! I tried to address your comments. |
|
🎊 +1 overall
This message was automatically generated. |
|
@joshelser I had solved this by just creating the walsDir here if it didn't exist, is there a reason you decided to skip it instead of create the dir? If we just create the directory it will naturally be empty and skip the rest of the "replayWals" function that you added. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
No intentional reason :). It looks like |
|
Shy of this failure, I"m not sure why Jenkins is unhappy. Will wait to see if Zach would like me to update this before trying to retrigger QA. |
Apache9
approved these changes
Oct 11, 2021
| if (walFs.exists(walsDir)) { | ||
| replayWALs(conf, walFs, walRootDir, walsDir, regionInfo, serverName, replayEditsDir); | ||
| } else { | ||
| LOG.warn("UNEXPECTED: WAL directory for MasterRegion is missing." |
There was a problem hiding this comment.
Better use LOG.error here and say it may cause data loss? In general, removing directories directly outside the HBase control is a dangerous operation...
|
Let me fix Duo's suggestion and then merge. |
joshelser
force-pushed
the
26267-masterdata-wals-list-masterbranch
branch
from
3a8f867 to
8e7b55b
Compare
|
🎊 +1 overall
This message was automatically generated. |
Nope, I don't feel strongly. Feel free to merge once Duo signs off on the method naming. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
1 similar comment
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
1 similar comment
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
A bit strange... Github report there are no conflicts but our pre commit job can not apply the patch to master... @joshelser Mind rebasing here? Thanks. |
|
Any updates here? @joshelser |
|
Sorry, Duo. Yep, on it. |
We currently cause an error to be thrown by trying to list a non-existent directory. We see that the master region directory exists on the filesystem, but forget to make sure that the master region's WAL directory also exists before we try to list it.
joshelser
force-pushed
the
26267-masterdata-wals-list-masterbranch
branch
from
8e7b55b to
e1da6b5
Compare
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
asfgit
pushed a commit
that referenced
this pull request
Nov 16, 2021
…ist (#3679) We currently cause an error to be thrown by trying to list a non-existent directory. We see that the master region directory exists on the filesystem, but forget to make sure that the master region's WAL directory also exists before we try to list it.
asfgit
pushed a commit
that referenced
this pull request
Nov 16, 2021
…ist (#3679) We currently cause an error to be thrown by trying to list a non-existent directory. We see that the master region directory exists on the filesystem, but forget to make sure that the master region's WAL directory also exists before we try to list it.
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.
We currently cause an error to be thrown by trying to list a
non-existent directory. We see that the master region directory exists
on the filesystem, but forget to make sure that the master region's WAL
directory also exists before we try to list it.
I had to change the TestBase class because
create()would create a new HTU which ended up creating a new test-directory every time. When we have a new test directory, we always hit bootstrap() instead of open() (which is what had the bug).