Free up the resources when materializing the results as Frames#15032
Free up the resources when materializing the results as Frames#15032LakshSingla merged 5 commits intoapache:masterfrom
Conversation
| */ | ||
| public interface CloseableCursor extends Cursor, Closeable | ||
| { | ||
| static CloseableCursor cursorWithCloseable(Cursor cursor, Closeable closeable) |
There was a problem hiding this comment.
I would expect this interface to have a cursor which can be closed.
From this close call, it seems like the cursor is untouched but we want to close the rowWalker ?
| RowWalker<Object[]> rowWalker = new RowWalker<>(Sequences.simple(rows), rowAdapter); | ||
| return new RowBasedCursor<>( | ||
|
|
||
| RowWalker<Object[]> rowWalker = new RowWalker<>(rows, rowAdapter); |
There was a problem hiding this comment.
Since you need to close the RowWalker, why don't you change RowBasedCursor and add a closeable to it.
In its close method, call rowWalker.close().
There was a problem hiding this comment.
Revisiting this comment, it doesn't seem fair that the RowBasedCursor should close the rowWalker that was passed to it by the caller. The caller should close the rowWalker.
Either the method getCursorFromSequence should return a Pair<Cursor, Closeable> or I will rename CloseableCursor to CursorWithCloseable
There was a problem hiding this comment.
Lets return a pair<Cursor,Closeable> in that case.
cryptoe
left a comment
cryptoe
left a comment
There was a problem hiding this comment.
Changes LGTM. Thanks @LakshSingla
…e#15032) Refactor the code to clean up the result sequences when materializing the results as Frames
…e#15032) Refactor the code to clean up the result sequences when materializing the results as Frames
3 participants
Description
RowWalkerdoesn't explicitly clean up the resources over the sequence that it acquires. Therefore, queries acquiring resources like the merge buffers fail to clean up the resources after they are done with it. This PR refactors the code to clean up the result sequences when materializing the results as Frames.The original PR that added this feature had a failing test that we ignored for the time being since we weren't seeing it in testing, and assumed that it might have been an issue with the
TestBuffersclass, however after facing this again, and investigating it uncovered the reason why the resources weren't getting cleaned up.This PR has: