[CELEBORN-2210] When a flushBuffer consolidation OOM exception occurs… #3547
Conversation
…, support setting the Buffer for fileInfo.
xy2953396112
force-pushed
the
CELEBORN-2210
branch
from
eb8561e to
8b9242a
Compare
| flushBuffer.consolidate() | ||
| fileInfo.setBuffer(flushBuffer) | ||
| try { | ||
| flushBuffer.consolidate() |
There was a problem hiding this comment.
Is flushBuffer safe when call consolidate encounter OutOfMemoryError?
There was a problem hiding this comment.
Even if the consolidate() method fails, the flushBuffer object itself remains valid.When CompositeByteBuf encounters an OutOfMemoryError, it will not be corrupted; only the memory consolidation operation fails to complete. the data integrity of flushBuffer is guaranteed, and it can be safely used continuously.
There was a problem hiding this comment.
An OutOfMemoryError exception only occurs when the allocBuffer method is called in CompositeByteBuf#consolidate0. Before this, the internal data structure of CompositeByteBuf has not been modified, so even if consolidate throws an exception, the original buffer can still be used.
|
@TheodoreLx Please help review this PR. Thanks. |
| flushBuffer.consolidate() | ||
| fileInfo.setBuffer(flushBuffer) | ||
| try { | ||
| flushBuffer.consolidate() |
There was a problem hiding this comment.
An OutOfMemoryError exception only occurs when the allocBuffer method is called in CompositeByteBuf#consolidate0. Before this, the internal data structure of CompositeByteBuf has not been modified, so even if consolidate throws an exception, the original buffer can still be used.
|
Thanks. merge to main(v0.7.0) |
3 participants
…, support setting the Buffer for fileInfo.
What changes were proposed in this pull request?
When a flushBuffer consolidation OOM exception occurs, support setting the Buffer for fileInfo.
Why are the changes needed?
When a flushBuffer consolidation OOM exception occurs, the current logic does not allow setting the Buffer for fileInfo.
Does this PR resolve a correctness bug?
NO
Does this PR introduce any user-facing change?
NO
How was this patch tested?
CI