-
Notifications
You must be signed in to change notification settings - Fork 594
HDDS-9940. Use MappedByteBuffer in ChunkManagerDummyImpl #9649
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Gargi-jais11
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Russole for the patch.
Left some review comments below.
| if (ch.size() < newSize) { | ||
| ch.truncate(newSize); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here, the code uses ch.truncate(newSize) to expand the file when ch.size() < newSize, but truncate() only reduces file size - it cannot expand files.
According to Java API:> "If the given size is greater than or equal to the file's current size then the file is not modified."
When the file is smaller than newSize, truncate() does nothing, leaving the file unchanged. This causes silent failures when read requests exceed the current mapped buffer size.
- Scenario 1: File is Smaller, Need Smaller Size (< 1MB)
Example:
- Current file: 500 KB
- Request: Read 256 KB
newSize = max(1MB, 256KB) = 1MB(because of DEFAULT_MAP_SIZE)
Current: ch.size() = 500 KB
Need: newSize = 1 MB
Check: Is 500 KB < 1 MB? → YES
Action: ch.truncate(1 MB)
Result: ❌ truncate() does NOTHING (can't expand)
File stays 500 KB, but we need 1 MB!
Problem: File doesn't expand to 1 MB, mapping fails or maps wrong size.
- Scenario 2: File is Larger, Need Smaller Size (< 1MB)
Example:
- Current file: 2 MB
- Request: Read 256 KB
newSize = max(1MB, 256KB) = 1MB
Current: ch.size() = 2 MB
Need: newSize = 1 MB
Check: Is 2 MB < 1 MB? → NO ❌
Action: Skip truncate (condition false)
Result: File is already big enough (2 MB > 1 MB)
But we're mapping only 1 MB, so it works!
Status: Works (but wastes space - file is 2 MB but we only use 1 MB)
We need to take care of these scenarios.
if (ch.size() < newSize) {
// LOGIC: Need to EXPAND file
} else if (ch.size() > newSize) {
// Need to SHRINK file
ch.truncate(newSize); // This works for shrinking!
}
|
Add some test coverage in
|
|
Thanks @Gargi-jais11 for the detailed review. I’ll make the necessary updates based on the comments. |
|
realistically ChunkManagerDummyImpl is for perf test only not production code. IMO doesn't require super rigorous test corner case coverage. (Though it's nothing if AI generates the test code) |
What changes were proposed in this pull request?
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-9940
How was this patch tested?
All CI checks passed.
https://github.com/Russole/ozone/actions/runs/21101732901