HDFS-17049. EC: Fix duplicate block group IDs generated by SequentialBlockGroupIdGenerator. #5743
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
…BlockGroupIdGenerator.
Hexiaoqiao
reviewed
Jun 14, 2023
| @@ -51,7 +51,7 @@ public class SequentialBlockGroupIdGenerator extends SequentialNumber { | |||
| } | |||
|
|
|||
| @Override // NumberGenerator | |||
| public long nextValue() { | |||
| synchronized public long nextValue() { | |||
There was a problem hiding this comment.
synchronized public long nextValue() -> public synchronized long nextValue()
| for (Thread t : threads) { | ||
| t.join(); | ||
| } | ||
| Set<Long> allBlockIds = new HashSet<>(); |
There was a problem hiding this comment.
Here will be more readable?
Set<Long> allBlockIds = new HashSet<>();
for (List<Long> set : blockIds) {
for (long id : set) {
if (!allBlockIds.add(id)) {
fail("Same block group id is generated!");
}
}
}
Any more simple check way? Such as using HashMap directly?
There was a problem hiding this comment.
Sorry I'm not understand how to use HashMap to get a more simple implement. I just modified the code as you did above.
|
@zhangshuyan0 Thank you very much for your valuable time. I would like to inquire about the concurrency issue in SequentialBlockGroupIdGenerator#nextValue. It seems that FSNamesystem#nextBlockId verifies FSNamesystemLock#writeLock. Block createNewBlock(BlockType blockType) throws IOException {
assert hasWriteLock();
Block b = new Block(nextBlockId(blockType), 0, 0);
// Increment the generation stamp for every new block.
b.setGenerationStamp(nextGenerationStamp(false));
return b;
} |
|
🎊 +1 overall
This message was automatically generated. |
|
@zhtttylz Thanks for your review. I met this problem because in our production code the directory locks have finer granularity, which means different files may obtain different locks. It looks that trunk code does not have this problem. Thanks again. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
Quite surprised to see how this can happen, we have write lock while getting nextBlockID |
cc @umamaheswararao FYI. |
|
Back to this PR. Considering that blockId/blockGroupId generator is protected by global lock, I think we do not need more |
|
@Hexiaoqiao Thanks for your suggestion. Could you please help close this PR and related JIRA? |
|
Got it. Close the PR since it does not need to fix. |
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
When I used multiple clients to write EC files concurrently, I found that NameNode generated the same block group ID for different files:
After diving into
SequentialBlockGroupIdGenerator, I found that the current implementation ofnextValueis not thread-safe.This problem must be fixed.
How was this patch tested?
Add a new unit test.