HDFS-17453. IncrementalBlockReport can have race condition with Edit Log Tailer #6708
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
|
I am planning to add another unit test that simulates this race condition. |
goiri
reviewed
Apr 4, 2024
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
@goiri I have added the unit test which reproduces the exact race condition. I confirmed this by testing the unit test against a branch without the fix and caught the false corrupt replicas. |
|
@sodonnel Could you take a look at my PR? I think it addresses the issue you mentioned here https://issues.apache.org/jira/browse/HDFS-15422?focusedCommentId=17287194&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-17287194 |
|
@kihwal Could you take a look at my PR? I think it addresses the issue you mentioned here https://issues.apache.org/jira/browse/HDFS-14941?focusedCommentId=17140156&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-17140156 |
goiri
approved these changes
Apr 6, 2024
| spies.add(nnSpy); | ||
| } | ||
|
|
||
| Thread.sleep(1000); |
| // the old reported block will be processed and marked as corrupt by the ANN. | ||
| // See HDFS-17453 | ||
| int size = queue.size(); | ||
| if (queue.removeIf(rbi -> rbi.storageInfo.equals(storageInfo) && |
There was a problem hiding this comment.
We could make this more robus to nulls with:
void enqueueReportedBlock(DatanodeStorageInfo storageInfo, Block block,
ReplicaState reportedState) {
if (storageInfo == null || block == null || reportedState == null) {
return;
}
...
if (queue.removeIf(rbi -> storageInfo.equals(rbi.storageInfo) &&
...
There was a problem hiding this comment.
I added these null checks, but I needed to remove the reportedState null check because the null value is used by removeStoredBlock
|
💔 -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. |
|
@dannytbecker thanks for your work, I'm just wondering what's the case of three GS with one block |
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.
Description of PR
Summary
There is a race condition between IncrementalBlockReports (IBR) and EditLogTailer in Standby NameNode (SNN) which can lead to leaked IBRs and false corrupt blocks after HA Failover. The race condition occurs when the SNN loads the edit logs before it receives the block reports from DataNode (DN).
Example
In the following example there is a block (b1) with 3 generation stamps (gs1, gs2, gs3).
If the example above happens for every DN which stores b1, then when the HA failover happens, b1 will be incorrectly marked as corrupt. This will be fixed when the first DN sends a FullBlockReport or an IBR for b1.
Logs from Active Cluster
I added the following logs to confirm this issue in an active cluster:
Logs from nn1 (Active):
Logs from nn2 (Standby):
Logs from nn2 when it transitions to Active:
How was this patch tested?
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?