From 77c19b7868833d211284ab88d3281edee9752e6e Mon Sep 17 00:00:00 2001 From: Deepak Kumar Date: Sat, 2 May 2026 23:35:42 -0700 Subject: [PATCH 1/3] fix(segment): close removed PinotDataBuffer in FilePerIndexDirectory.removeIndex --- .../segment/store/FilePerIndexDirectory.java | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/store/FilePerIndexDirectory.java b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/store/FilePerIndexDirectory.java index fe05e2e49605..38559ec7dedb 100644 --- a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/store/FilePerIndexDirectory.java +++ b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/store/FilePerIndexDirectory.java @@ -36,9 +36,13 @@ import org.apache.pinot.segment.spi.memory.PinotDataBuffer; import org.apache.pinot.segment.spi.store.ColumnIndexDirectory; import org.apache.pinot.spi.utils.ReadMode; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; class FilePerIndexDirectory extends ColumnIndexDirectory { + private static final Logger LOGGER = LoggerFactory.getLogger(FilePerIndexDirectory.class); + private final File _segmentDirectory; private SegmentMetadataImpl _segmentMetadata; private final ReadMode _readMode; @@ -99,8 +103,15 @@ public void close() @Override public void removeIndex(String columnName, IndexType indexType) { - // TODO: this leaks the removed data buffer (it's not going to be freed in close() method) - _indexBuffers.remove(new IndexKey(columnName, indexType)); + PinotDataBuffer removedBuffer = _indexBuffers.remove(new IndexKey(columnName, indexType)); + if (removedBuffer != null) { + try { + removedBuffer.close(); + } catch (Exception e) { + LOGGER.warn("Failed to close removed buffer for column: {}, indexType: {}", + columnName, indexType, e); + } + } if (indexType == StandardIndexes.text()) { TextIndexUtils.cleanupTextIndex(_segmentDirectory, columnName); } else if (indexType == StandardIndexes.vector()) { From 44afbfaa43acc24f577ffc27d84da71d899e652a Mon Sep 17 00:00:00 2001 From: Deepak kumar Date: Tue, 5 May 2026 16:44:17 -0700 Subject: [PATCH 2/3] Apply suggestion from @xiangfu0 Co-authored-by: Xiang Fu --- .../segment/store/FilePerIndexDirectory.java | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/store/FilePerIndexDirectory.java b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/store/FilePerIndexDirectory.java index 38559ec7dedb..78e4aa99bed7 100644 --- a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/store/FilePerIndexDirectory.java +++ b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/store/FilePerIndexDirectory.java @@ -103,13 +103,18 @@ public void close() @Override public void removeIndex(String columnName, IndexType indexType) { - PinotDataBuffer removedBuffer = _indexBuffers.remove(new IndexKey(columnName, indexType)); - if (removedBuffer != null) { + IndexKey key = new IndexKey(columnName, indexType); + // Fetch the buffer without removing it from the map first; the mapping is removed only after close() succeeds. + // If close() throws, the entry stays in _indexBuffers so a subsequent directory close() can attempt to release + // it on a best-effort basis (this class is not thread-safe; callers must serialize access). + PinotDataBuffer buffer = _indexBuffers.get(key); + if (buffer != null) { try { - removedBuffer.close(); + buffer.close(); + _indexBuffers.remove(key); } catch (Exception e) { - LOGGER.warn("Failed to close removed buffer for column: {}, indexType: {}", - columnName, indexType, e); + throw new RuntimeException( + String.format("Failed to close index buffer for column: %s, indexType: %s", columnName, indexType), e); } } if (indexType == StandardIndexes.text()) { From c42fa0f6290490bef191590286a1061c093b7c85 Mon Sep 17 00:00:00 2001 From: Deepak Kumar Date: Thu, 7 May 2026 12:42:33 -0700 Subject: [PATCH 3/3] Remove unused LOGGER per review feedback from @Jackie-Jiang --- .../segment/local/segment/store/FilePerIndexDirectory.java | 4 ---- 1 file changed, 4 deletions(-) diff --git a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/store/FilePerIndexDirectory.java b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/store/FilePerIndexDirectory.java index 78e4aa99bed7..a597ef1aabca 100644 --- a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/store/FilePerIndexDirectory.java +++ b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/store/FilePerIndexDirectory.java @@ -36,13 +36,9 @@ import org.apache.pinot.segment.spi.memory.PinotDataBuffer; import org.apache.pinot.segment.spi.store.ColumnIndexDirectory; import org.apache.pinot.spi.utils.ReadMode; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; class FilePerIndexDirectory extends ColumnIndexDirectory { - private static final Logger LOGGER = LoggerFactory.getLogger(FilePerIndexDirectory.class); - private final File _segmentDirectory; private SegmentMetadataImpl _segmentMetadata; private final ReadMode _readMode;