HBASE-25932: Ensure replication reads the trailer bytes from WAL. #3332
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
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
Test failures are not related, looped them over 100 times and they passed locally (failed-to-read.. is a known issue). |
virajjasani
approved these changes
May 29, 2021
...erver/src/test/java/org/apache/hadoop/hbase/replication/regionserver/TestWALEntryStream.java
Show resolved
Hide resolved
Comment on lines
124
to
126
| Waiter.waitFor(CONF, TEST_TIMEOUT_MS, (Waiter.Predicate<Exception>) () -> (result | ||
| = WALEntryStreamWithRetries.super.next()) != null); |
There was a problem hiding this comment.
nit:
Waiter.waitFor(CONF, TEST_TIMEOUT_MS, () -> (result = WALEntryStreamWithRetries.super.next()) != null);
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
Nice find
FSHLog also been closed in async way in Master branch. Pls see FSHLog#doReplaceWriter. No applicable there? I have not seen the original issue description though! |
@anoopsjohn Thanks for pointing it out, I incorrectly assumed that FSHLog behavior stayed the same in branch-2/master. This async closing of FSHLog was not ported to branch-1. On top of that branch-2/master tests have coverage only for AsyncWAL since that is the default and that didn't help. The latest patch fixes this issue for both the implementations along with test coverage. PTAL. |
bharathv
changed the title
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
anoopsjohn
reviewed
May 31, 2021
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/AbstractFSWAL.java
Show resolved
Hide resolved
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
This bug was exposed by the test from HBASE-25924. Since this wal implementations close the wal asynchronously, replication can potentially miss the trailer bytes. (see jira comment for detailed analysis). While this is not a correctness problem (since trailer does not have any entry data), it erroneously bumps a metric that is used to track skipped bytes in WAL resulting in false alarms which is something we should avoid.
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
anoopsjohn
approved these changes
Jun 1, 2021
| @@ -530,7 +557,8 @@ public void testReplicationSourceWALReaderWrongPosition() throws Exception { | |||
|
|
|||
| @Override | |||
| public boolean evaluate() throws Exception { | |||
| return fs.getFileStatus(walPath).getLen() > 0; | |||
| return fs.getFileStatus(walPath).getLen() > 0 && | |||
| ((AbstractFSWAL) log).getInflightWALCloseCount() == 0; | |||
There was a problem hiding this comment.
We wait for 5 sec and check. Should be wait for getInflightWALCloseCount to be zero before coming here? Or will that be a better guarantee for WAL close has happened where we come here for FS.getLen() check?
There was a problem hiding this comment.
We don't wait for 5s and check. We wait for the condition to pass within 5s or timeout, this is the javadoc, think the test is doing what you are saying (unless I misunderstood what you are saying)..
/**
* Waits up to the duration equal to the specified timeout multiplied by the
* {@link #getWaitForRatio(Configuration)} for the given {@link Predicate} to become
* <code>true</code>, failing the test if the timeout is reached and the Predicate is still
* <code>false</code>.
* <p/>
* @param conf the configuration
* @param timeout the timeout in milliseconds to wait for the predicate.
* @param predicate the predicate to evaluate.
* @return the effective wait, in milli-seconds until the predicate becomes <code>true</code> or
* wait is interrupted otherwise <code>-1</code> when times out
*/
| /** | ||
| * Test helper that waits until a non-null entry is available in the stream next or times out. | ||
| */ | ||
| private static class WALEntryStreamWithRetries extends WALEntryStream { |
There was a problem hiding this comment.
Can u also pls explain why we use this special subclass here for the tests? (as comments) will be easy to read and digest later.
|
Oops, I thought I had a +1 from you @anoopsjohn and I merged the change. Let me open a follow up patch to address your comments. |
bharathv
added a commit
to bharathv/hbase
that referenced
this pull request
Jun 1, 2021
…ache#3332) This bug was exposed by the test from HBASE-25924. Since this wal implementations close the wal asynchronously, replication can potentially miss the trailer bytes. (see jira comment for detailed analysis). While this is not a correctness problem (since trailer does not have any entry data), it erroneously bumps a metric that is used to track skipped bytes in WAL resulting in false alarms which is something we should avoid. Reviewed-by: Rushabh Shah <[email protected]> Signed-off-by: Viraj Jasani <[email protected]> Signed-off-by Anoop Sam John <[email protected]> (cherry picked from commit b04c3c7)
bharathv
added a commit
to bharathv/hbase
that referenced
this pull request
Jun 1, 2021
…ache#3332) This bug was exposed by the test from HBASE-25924. Since this wal implementations close the wal asynchronously, replication can potentially miss the trailer bytes. (see jira comment for detailed analysis). While this is not a correctness problem (since trailer does not have any entry data), it erroneously bumps a metric that is used to track skipped bytes in WAL resulting in false alarms which is something we should avoid. Reviewed-by: Rushabh Shah <[email protected]> Signed-off-by: Viraj Jasani <[email protected]> Signed-off-by Anoop Sam John <[email protected]> (cherry picked from commit b04c3c7)
bharathv
added a commit
that referenced
this pull request
Jun 1, 2021
) (#3346) This bug was exposed by the test from HBASE-25924. Since this wal implementations close the wal asynchronously, replication can potentially miss the trailer bytes. (see jira comment for detailed analysis). While this is not a correctness problem (since trailer does not have any entry data), it erroneously bumps a metric that is used to track skipped bytes in WAL resulting in false alarms which is something we should avoid. Reviewed-by: Rushabh Shah <[email protected]> Signed-off-by: Viraj Jasani <[email protected]> Signed-off-by Anoop Sam John <[email protected]> (cherry picked from commit b04c3c7)
bharathv
added a commit
that referenced
this pull request
Jun 1, 2021
) (#3345) This bug was exposed by the test from HBASE-25924. Since this wal implementations close the wal asynchronously, replication can potentially miss the trailer bytes. (see jira comment for detailed analysis). While this is not a correctness problem (since trailer does not have any entry data), it erroneously bumps a metric that is used to track skipped bytes in WAL resulting in false alarms which is something we should avoid. Reviewed-by: Rushabh Shah <[email protected]> Signed-off-by: Viraj Jasani <[email protected]> Signed-off-by Anoop Sam John <[email protected]> (cherry picked from commit b04c3c7)
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.
This bug was exposed by the test from HBASE-25924. Since this wal
implementations close the wal asynchronously, replication can potentially
miss the trailer bytes. (see jira comment for detailed analysis).
While this is not a correctness problem (since trailer does not have any entry data),
it erroneously bumps a metric that is used to track skipped bytes in WAL resulting
in false alarms which is something we should avoid.