HBASE-26106 AbstractFSWALProvider#getArchivedLogPath doesn't look for wal file in all oldWALs directory. #3636
Conversation
This file contains 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
added 2 commits
August 27, 2021 12:38
… wal file in all oldWALs directory.
shahrs87
commented
Aug 27, 2021
| * @param conf configuration | ||
| * @return WAL Reader instance | ||
| */ | ||
| public static org.apache.hadoop.hbase.wal.WAL.Reader openReader(Path path, Configuration conf) |
There was a problem hiding this comment.
This method is not called anywhere so removing it.
shahrs87
commented
Aug 27, 2021
| } | ||
|
|
||
| // For HBASE-15019 | ||
| private static void recoverLease(final Configuration conf, final Path path) { |
There was a problem hiding this comment.
This method is not called anywhere so removing it.
|
💔 -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. |
Apache9
approved these changes
Aug 28, 2021
|
OK, so the methods are used in other modules, like hbase-mapreduce? |
Yes, somehow Intellij was misleading me. Fixed it in last version. |
shahrs87
commented
Aug 30, 2021
| assertTrue(fs.rename(logFile, archivedLogLocation)); | ||
| assertTrue(fs.exists(archivedLogDir)); | ||
| assertFalse(fs.exists(logFile)); | ||
| // TODO: This is not behaving as expected. WALInputFormat#WALKeyRecordReader doesn't open |
There was a problem hiding this comment.
This test is not testing what it is intending to.
Before this patch, it was using AbstractFSWALProvider.getArchivedLogPath(logFile, conf); to get archivedLogLocation. But AbstractFSWALProvider#getArchivedLogPath was returning the same path as logFile from here
After this patch, we are able to successfully rename the file to oldWALs directory but somehow it is not triggering the condition within nextKeyValue method to look into archiveDir.
There was a problem hiding this comment.
So you mean we should file another issue to address this? Anyway, I think this PR is a critical one as it could cause data loss, so let me merge it to all branches first in case of not blocking the upcoming 2.4.x release.
There was a problem hiding this comment.
I was going to cut 2.4.6RC0 today but saw this issue. Will wait until this is merged to branch-2.4.
There was a problem hiding this comment.
So you mean we should file another issue to address this
Yes, we need to open a new jira to modify the test. Will file it soon.
There was a problem hiding this comment.
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
@Apache9 Could you please review again ? Thank you ! |
|
🎊 +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. |
apurtell
requested changes
Aug 31, 2021
hbase-mapreduce/src/main/java/org/apache/hadoop/hbase/mapreduce/WALInputFormat.java
Outdated
Show resolved
Hide resolved
hbase-server/src/main/java/org/apache/hadoop/hbase/wal/AbstractFSWALProvider.java
Outdated
Show resolved
Hide resolved
|
I left a 'Changes Requested' review but if the comment is addressed at commit time we don't need another round of review from me. |
|
🎊 +1 overall
This message was automatically generated. |
|
Please fix the checkstyle issue? Thanks. |
Fixed. It was a new warning after I removed file comparison. |
apurtell
approved these changes
Sep 1, 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. |
|
🎊 +1 overall
This message was automatically generated. |
asfgit
pushed a commit
that referenced
this pull request
Sep 2, 2021
… wal file in all oldWALs directory. (#3636) Signed-off-by: Andrew Purtell <apurtell@apache.org> Signed-off-by: Duo Zhang <zhangduo@apache.org> Conflicts: hbase-mapreduce/src/test/java/org/apache/hadoop/hbase/mapreduce/TestWALRecordReader.java
asfgit
pushed a commit
that referenced
this pull request
Sep 2, 2021
… wal file in all oldWALs directory. (#3636) Signed-off-by: Andrew Purtell <apurtell@apache.org> Signed-off-by: Duo Zhang <zhangduo@apache.org> Conflicts: hbase-mapreduce/src/test/java/org/apache/hadoop/hbase/mapreduce/TestWALRecordReader.java
guangxuCheng
pushed a commit
to guangxuCheng/hbase
that referenced
this pull request
Mar 28, 2022
… wal file in all oldWALs directory. (apache#3636)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.