-
Notifications
You must be signed in to change notification settings - Fork 996
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
PHOENIX-6342 RoundRobinResultIterator doesn't close itself when resul… #1114
Conversation
…t size = fetchsize-1
@@ -114,6 +114,7 @@ public Tuple next() throws SQLException { | |||
index = (index + 1) % size; | |||
} | |||
} | |||
close(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When RRIterator#peek() (next result) is null, we close the iterator. But if we don't get any next result after looping through all RRIterators, we should close() here, that's the plan right?
Although this looks good, curious if caller can implement the logic of calling RRResultIterator#close() if RRResultIterator#next() returns null. But if caller logic turns out to be more complicated, sure we should go this route I believe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems we already do have practice of closing iterators when next() exhausts all rows e.g ConcatResultIterator
, LimitingResultIterator
. I think we are good here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When RRIterator#peek() (next result) is null, we close the iterator. But if we don't get any next result after looping through all RRIterators, we should close() here, that's the plan right?
Yes. When all iterators set numScannersCacheExhausted , and we're at the end, getIterators() will return an empty collection, and close() is skipped.
Although this looks good, curious if caller can implement the logic of calling RRResultIterator#close() if RRResultIterator#next() returns null. But if caller logic turns out to be more complicated, sure we should go this route I believe.
This is is the pattern everywhere in the code. This is also less error-prone, and encapsulates the logic within the class, I see no reason to change this, either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 (non-binding)
💔 -1 overall
This message was automatically generated. |
…t size = fetchsize-1