Skip to content

PHOENIX-6342 RoundRobinResultIterator doesn't close itself when resul…#1114

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.

Sign up for GitHub

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

Closed
stoty wants to merge 1 commit into from
Closed

PHOENIX-6342 RoundRobinResultIterator doesn't close itself when resul… #1114

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions phoenix-core/src/main/java/org/apache/phoenix/iterate/RoundRobinResultIterator.java
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -114,6 +114,7 @@ public Tuple next() throws SQLException {
index = (index + 1) % size;
}
}
close();

@virajjasanivirajjasaniJan 27, 2021

Copy link
Copy Markdown
Contributor

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.

@virajjasanivirajjasaniJan 27, 2021

Copy link
Copy Markdown
Contributor

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.

@stotystotyJan 27, 2021•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

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.

return null;
}

Expand Down