Jackie-Jiang commented on code in PR #15139:
URL: https://github.com/apache/pinot/pull/15139#discussion_r1984381792
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/readers/forward/VarByteChunkForwardIndexReaderV4.java:
##########
@@ -445,6 +444,7 @@ private void decompressViaDirectBuffer(ByteBuffer
compressed, byte[] target)
@Override
public void close() {
CleanerUtil.cleanQuietly(_decompressedBuffer);
+ _decompressedBuffer = null;
Review Comment:
Do we need to set it to `null` after cleaning it?
##########
pinot-core/src/main/java/org/apache/pinot/core/common/DataFetcher.java:
##########
@@ -594,4 +594,17 @@ public void close()
}
}
}
+
+ /**
+ * Close the DataFetcher and release all resources (specifically, the
ForwardIndexReaderContext off-heap buffers).
+ */
+ public void close() {
+ try {
+ for (ColumnValueReader columnValueReader :
_columnValueReaderMap.values()) {
+ columnValueReader.close();
+ }
+ } catch (IOException e) {
+ // do nothing
Review Comment:
Same for other places
##########
pinot-core/src/main/java/org/apache/pinot/core/common/DataFetcher.java:
##########
@@ -594,4 +594,17 @@ public void close()
}
}
}
+
+ /**
+ * Close the DataFetcher and release all resources (specifically, the
ForwardIndexReaderContext off-heap buffers).
+ */
+ public void close() {
+ try {
+ for (ColumnValueReader columnValueReader :
_columnValueReaderMap.values()) {
+ columnValueReader.close();
+ }
+ } catch (IOException e) {
+ // do nothing
Review Comment:
Should we throw the exception out? Swallow exception silently is not good
practice unless it is expected
##########
pinot-core/src/main/java/org/apache/pinot/core/common/DataBlockCache.java:
##########
@@ -406,4 +406,11 @@ private void putValues(FieldSpec.DataType dataType, String
column, Object values
public void addDataSource(String fullColumnKeyName, DataSource
keyDataSource) {
_dataFetcher.addDataSource(fullColumnKeyName, keyDataSource);
}
+
+ /**
+ * Close the data block cache and release all resources.
+ */
+ public void close() {
Review Comment:
Make these classes implement `AutoCloseable` so that they can be used within
try-with-resource
--
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]
