Copilot commented on code in PR #10353:
URL: https://github.com/apache/ozone/pull/10353#discussion_r3401555699
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/helpers/ChunkUtils.java:
##########
@@ -253,16 +260,23 @@ private static void readData(File file, long offset, long
len,
* @return a list of {@link MappedByteBuffer} containing the data.
*/
private static ChunkBuffer readData(File file, int chunkSize,
- long offset, long length, HddsVolume volume, MappedBufferManager
mappedBufferManager)
+ long offset, long length, HddsVolume volume, MappedBufferManager
mappedBufferManager,
+ DispatcherContext dispatcherContext)
throws StorageContainerException {
final int bufferNum = Math.toIntExact((length - 1) / chunkSize) + 1;
if (!mappedBufferManager.getQuota(bufferNum)) {
// proceed with normal buffer
- final ByteBuffer[] buffers = BufferUtils.assignByteBuffers(length,
- chunkSize);
+ final ByteBuffer[] buffers = BufferUtils.assignByteBuffers(length,
chunkSize);
readData(file, offset, length, c -> c.position(offset).read(buffers),
volume);
Arrays.stream(buffers).forEach(ByteBuffer::flip);
+ if (dispatcherContext != null && dispatcherContext.isReleaseSupported())
{
+ dispatcherContext.setReleaseMethod(() -> {
Review Comment:
In the mmap quota fallback branch (normal FileChannel read into pooled
direct buffers), buffers are returned to the pool only on the success path. If
the read fails, the allocated direct buffers are not returned, which can leak
direct memory. Register the releaser immediately after allocation and run it on
exceptions when release isn’t registered.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/helpers/ChunkUtils.java:
##########
@@ -202,18 +202,25 @@ private static long writeDataToChannel(FileChannel
channel, ChunkBuffer data,
@SuppressWarnings("checkstyle:parameternumber")
public static ChunkBuffer readData(long len, int bufferCapacity,
File file, long off, HddsVolume volume, int readMappedBufferThreshold,
boolean mmapEnabled,
- MappedBufferManager mappedBufferManager) throws
StorageContainerException {
+ MappedBufferManager mappedBufferManager, DispatcherContext
dispatcherContext)
+ throws StorageContainerException {
if (mmapEnabled && len > readMappedBufferThreshold && bufferCapacity >
readMappedBufferThreshold) {
- return readData(file, bufferCapacity, off, len, volume,
mappedBufferManager);
+ return readData(file, bufferCapacity, off, len, volume,
mappedBufferManager, dispatcherContext);
} else if (len == 0) {
return ChunkBuffer.wrap(Collections.emptyList());
+ } else {
+ final ByteBuffer[] buffers = BufferUtils.assignByteBuffers(len,
bufferCapacity);
+ readData(file, off, len, c -> c.position(off).read(buffers), volume);
+ Arrays.stream(buffers).forEach(ByteBuffer::flip);
+ if (dispatcherContext != null && dispatcherContext.isReleaseSupported())
{
+ dispatcherContext.setReleaseMethod(() -> {
Review Comment:
Direct buffers allocated from the pool are only returned via
dispatcherContext.release() on the success path. If readData(...) throws (e.g.,
I/O error) before setReleaseMethod is reached, the allocated direct buffers are
never returned to the pool, which can leak direct memory under repeated
failures. Register the release method immediately after allocation, and ensure
buffers are returned in a catch/finally path when release isn’t registered.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
