HDFS-17800 Prevent observer node from returning partial block data for erasure coded files#7754
Open
Kimahriman wants to merge 4 commits intoapache:trunkfrom
Open
HDFS-17800 Prevent observer node from returning partial block data for erasure coded files#7754Kimahriman wants to merge 4 commits intoapache:trunkfrom
Kimahriman wants to merge 4 commits intoapache:trunkfrom
Conversation
Kimahriman
changed the title
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
Kimahriman
force-pushed
the
observer-invalid-ec-blocks
branch
from
ced8b93 to
9acfff1
Compare
|
🎊 +1 overall
This message was automatically generated. |
Contributor
Author
|
@jojochuang @xkrogen since you have reviewed similar fixes in the past |
|
🎊 +1 overall
This message was automatically generated. |
Kimahriman
force-pushed
the
observer-invalid-ec-blocks
branch
from
9acfff1 to
832aae2
Compare
|
🎊 +1 overall
This message was automatically generated. |
Kimahriman
force-pushed
the
observer-invalid-ec-blocks
branch
from
832aae2 to
f738948
Compare
|
🎊 +1 overall
This message was automatically generated. |
Kimahriman
force-pushed
the
observer-invalid-ec-blocks
branch
from
f738948 to
ba7594f
Compare
|
💔 -1 overall
This message was automatically generated. |
Kimahriman
force-pushed
the
observer-invalid-ec-blocks
branch
from
ba7594f to
9cacb5c
Compare
|
💔 -1 overall
This message was automatically generated. |
Kimahriman
force-pushed
the
observer-invalid-ec-blocks
branch
from
9cacb5c to
6d4b2f4
Compare
Kimahriman
force-pushed
the
observer-invalid-ec-blocks
branch
from
6d4b2f4 to
8db70bc
Compare
|
🎊 +1 overall
This message was automatically generated. |
Contributor
|
It would be better to extract the logic for calculating minBlocks into a unified method, for example named |
Contributor
Author
Good call, updated for both |
|
🎊 +1 overall
This message was automatically generated. |
Contributor
|
Great job, after further thinking, when check EC data in |
Contributor
Author
|
Ah yeah didn't realize the indices could be duplicated, addressed that as well |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
Contributor
|
Looks good to me! |
CapMoon
approved these changes
Apr 24, 2026
ZanderXu
reviewed
Apr 24, 2026
Contributor
ZanderXu
left a comment
ZanderXu
left a comment
There was a problem hiding this comment.
Thanks @Kimahriman for your contribution.
Make sense to me.
Maybe you can split this MR to two MRs:
- The first MR correct EC checks for ObserverRead
- The second MR correct EC checks for safeMode, since I saw that some other methods missed safemode check, such as: getFileInfo. Maybe you can sort out all related cases and fix them together.
ZanderXu
reviewed
Apr 24, 2026
| } | ||
|
|
||
| if (block.isStriped()) { | ||
| LocatedStripedBlock stripedBlock = (LocatedStripedBlock) block; |
Contributor
There was a problem hiding this comment.
LocatedStripedBlock stripedBlock = (LocatedStripedBlock) block;
// For erasure coded files, require enough data units to reconstruct
// the block, bounded by the number of cells in the block.
long numCells = (block.getBlockSize() - 1) / ecPolicy.getCellSize() + 1;
int minRequiredIndices = (int) Math.min(ecPolicy.getNumDataUnits(), numCells);
// Units can be over-replicated, so need to account for unique indices
byte[] blockIndices = stripedBlock.getBlockIndices();
boolean[] seenIndices = new boolean[ecPolicy.getNumDataUnits() + ecPolicy.getNumParityUnits()];
int uniqueIndexCount = 0;
for (byte idx : blockIndices) {
int index = idx & 0xFF;
if (!seenIndices[index]) {
seenIndices[index] = true;
if (++uniqueIndexCount >= minRequiredIndices) {
return true;
}
}
}
return false;
| return haEnabled && haContext != null && haContext.getState().getServiceState() == OBSERVER; | ||
| } | ||
|
|
||
| private boolean hasSufficientReplicas(LocatedBlock block, |
Contributor
There was a problem hiding this comment.
hasSufficientBlockLocations
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 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.
Description of PR
HDFS-17800
Follow up to HDFS-13924, HDFS-16732, and HDFS-17768, factor in erasure coding policy when determining if observer or safe mode nodes have enough blocks to successfully serve a request, as a single block may not be sufficient for an erasure coded block.
How was this patch tested?
Updated existing UTs to factor in erasure coding policy.
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?