HBASE-26093 Replication is stuck due to zero length wal file in oldWALs directory #3504
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. |
apurtell
approved these changes
Jul 19, 2021
|
🎊 +1 overall
This message was automatically generated. |
bharathv
approved these changes
Jul 19, 2021
Comment on lines
172
to
178
| try { | ||
| if (entryStream != null) { | ||
| entryStream.close(); | ||
| } | ||
| } catch (IOException ioe) { | ||
| LOG.warn("Exception while closing WALEntryStream", ioe); | ||
| } |
Contributor
There was a problem hiding this comment.
Can be simplified as follows,
IOUtils.closeQuietly(entryStream, e -> LOG.warn("Exception while closing WALEntryStream", e));
| @Test | ||
| public void testEOFExceptionInOldWALsDirectory() throws Exception { | ||
| assertEquals(1, logQueue.getQueueSize(fakeWalGroupId)); | ||
| AbstractFSWAL abstractWAL = (AbstractFSWAL)log; |
Contributor
There was a problem hiding this comment.
nit: We don't need this down cast, do we?
Contributor
Author
There was a problem hiding this comment.
@bharathv It is needed. We are getting the current wal file name via AbstractFSWAL#getCurrentFileName so that we can truncate that wal file to create 0 size wal file. WAL class doesn't have this method.
Contributor
Author
|
🎊 +1 overall
This message was automatically generated. |
Apache9
requested changes
Jul 20, 2021
| } | ||
|
|
||
| private Path getArchivedLog(Path path) throws IOException { | ||
| Path getArchivedLog(Path path) throws IOException { |
Contributor
There was a problem hiding this comment.
Let's move this method to a util class so we do not need to change the modifier and also do not need to pass the WALEntryStream to the handleEofException?
| } catch (InterruptedException e) { | ||
| LOG.trace("Interrupted while sleeping between WAL reads or adding WAL batch to ship queue"); | ||
| Thread.currentThread().interrupt(); | ||
| } finally { |
Contributor
There was a problem hiding this comment.
What is the advantage here to not use try-with-resources but a try finally?
Contributor
Author
There was a problem hiding this comment.
I want to use entryStream object in handleEofException method. If I use try-wth-resources, I don't have access to entryStream in catch block.
Contributor
There was a problem hiding this comment.
OK, so if we could move the main logic in WALEntryStream.getArchivedLog to a util class, then we do not need try finally then? In general, I do not think we should expose WALEntryStream.getArchivedLog directly. Let's add a public static method in AbstractFSWALProvider, just below the getWALArchiveDirectoryName method. I think this is the right place for holding a public method.
Contributor
Author
There was a problem hiding this comment.
Thank you @Apache9 for the review. Didn't know a utility method AbstractFSWALProvider#getArchivedLogPath already exists.
Removed all the changes to make WALEntryStream class. Please review again.
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
Apache9
requested changes
Jul 21, 2021
| LOG.warn("Forcing removal of 0 length log in queue: {}", head); | ||
| if (!fs.exists(path)) { | ||
| // There is a chance that wal has moved to oldWALs directory, so look there also. | ||
| path = AbstractFSWALProvider.getArchivedLogPath(path, conf); |
Contributor
There was a problem hiding this comment.
Oh, this is not very stable...
You need to try all the possible path, just like what we have done in WALEntryStream. So I suggest we move the main logic of WALEntryStream.getArchivedLog to AbstractFSWALProvider, as a util method, and we call this method in both places.
Contributor
Author
There was a problem hiding this comment.
Thank you @Apache9 for the feedback. I added a new util method in AbstractFSWALProvider to look into both the oldWALs path if it can't find the log in WALs directory.
After the change, there are 2 methods in AbstractFSWALProvider now to get archiveLog path:
getArchivedLogThis is the method added by this patch.getArchivedLogPathThis was already present which looks into separate oldLogDir path based on config.
I think we can remove getArchivedLogPath method and replace all callers with getArchivedLog. WDYT ?
Contributor
There was a problem hiding this comment.
Oh, just saw this comment. Please see my new comment below, you need to keep getArchivedLogPath, as it is used when we want to archive a wal file.
It is OK to remove the getArchivedLog in WALEntryStream.
Apache9
reviewed
Jul 21, 2021
| * @return archived path if exists, path - otherwise | ||
| * @throws IOException exception | ||
| */ | ||
| public static Path getArchivedLog(Path path, Configuration conf) throws IOException { |
Contributor
There was a problem hiding this comment.
Better name it findArchivedLog.
The above getArchivedLogPath is used when we want to move a file into the archived directory, we need to check the configuration to see if we want to put the log into the separate directory or not.
And for this method, it is used when we want to read a wal file, so we need to try all the places to see if we can find it, as we may change the way on how to archive the log on the fly(not sure but in general we want to support onConfigurationChange for more and more configurations).
So name it findArchivedLog will be better, and maybe we could just return null, or return an Optional object when we can not find the wal file.
Contributor
Author
There was a problem hiding this comment.
The above getArchivedLogPath is used when we want to move a file into the archived directory, we need to check the configuration to see if we want to put the log into the separate directory or not.
In production code, these 3 places are where we are calling getArchivedLogPath util method.
- AbstractFSWALProvider#openReader
- ReplicationSource#getFileSize
- WALInputFormat.WALRecordReader#nextKeyValue
@Apache9 All of the above calls are trying to find the log in archive path after they couldn't locate the wal in walsDir and they are not used for moving a log file to archive directory. Correct me if I am wrong.
AbstractFSWALProvider#getWALArchiveDirectoryName is the mehod we use to locate the wal archive directory to move the wal.
Contributor
There was a problem hiding this comment.
Good. I think you have found potential bugs in our code base. We should change them all to make use of the new method you introduced in this PR. Suggest opening a new PR to address the incorrect usages of getArchivedLogPath.
Contributor
Author
There was a problem hiding this comment.
Suggest opening a new PR to address the incorrect usages of getArchivedLogPath.
Created https://issues.apache.org/jira/browse/HBASE-26106 for this.
|
🎊 +1 overall
This message was automatically generated. |
Apache9
approved these changes
Jul 21, 2021
|
💔 -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
Author
TestReplicationKillMasterRSWithSeparateOldWALs is a valid failure. It is failing here with the following stack trace I printed the wal file name and it failed with NPE above. Interestingly the path contains oldWALs directory and it is trying to find archive log for oldWALs directory. Below is the method of findArchivedLog method @Apache9 could we exit |
|
💔 -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
Author
|
Ran both failing tests on local machine and succeeded @Apache9 could you please review. |
Contributor
|
Those test failures do not seem related. |
Contributor
|
Let's give 24 hours for more responses or activity here. Otherwise I will merge this and pick it back to branch-2.4 and proceed with the 2.4.5 RC tomorrow (Friday PST). |
asfgit
pushed a commit
that referenced
this pull request
Jul 23, 2021
…Ls directory (#3504) Signed-off-by: Andrew Purtell <apurtell@apache.org> Signed-off-by: Bharath Vissapragada <bharathv@apache.org> Signed-off-by: Duo Zhang <zhangduo@apache.org>
asfgit
pushed a commit
that referenced
this pull request
Jul 23, 2021
…Ls directory (#3504) Signed-off-by: Andrew Purtell <apurtell@apache.org> Signed-off-by: Bharath Vissapragada <bharathv@apache.org> Signed-off-by: Duo Zhang <zhangduo@apache.org>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 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.
No description provided.