Skip to content

ResultSet: handle empty non-final pages on ResultSet iteration - #102

Merged
fruch merged 1 commit into
masterfrom
fix_empty_paging
Aug 30, 2021
Merged

ResultSet: handle empty non-final pages on ResultSet iteration#102
fruch merged 1 commit into
masterfrom
fix_empty_paging

Conversation

@ultrabug

Copy link
Copy Markdown

This commit provides a fix to the situation when iterating on a
ResultSet, the driver aborts the iteration if the server returns
an empty page even if there are next pages available.

Python driver is affected by the same problem as JAVA-2934
This fix is similar to apache/cassandra-java-driver#1544

This commit provides a fix to the situation when iterating on a
ResultSet, the driver aborts the iteration if the server returns
an empty page even if there are next pages available.
Python driver is affected by the same problem as JAVA-2934
This fix is similar to apache/cassandra-java-driver#1544
Comment threadcassandra/cluster.py
if not self.response_future._continuous_paging_session:
self.fetch_next_page()
self._page_iter = iter(self._current_rows)
return self.next()

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ultrabug this is identical to the next line after the if, how this fixes anything ?, do we have a test case to demonstrate this ?

@haaawkhaaawkAug 30, 2021

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not doing the same. It runs the whole method again (recursively). next(self) not only calls next(self._page_iter) but also has all the logic in lines 5127-5137.

@haaawk

Copy link
Copy Markdown

Would it be inappropriate to ask for a unit test?

@fruchfruch left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@fruch
fruch merged commit 6ed53d9 into masterAug 30, 2021
@ultrabug

Copy link
Copy Markdown
Author

Thanks @fruch

Would it be inappropriate to ask for a unit test?

I'll try to add this on a second PR okay, first of all I need to make sure the current tests do fail because paging is supposed to be tested already...

@nyh

nyh commented Oct 11, 2021

Copy link
Copy Markdown

Would it be inappropriate to ask for a unit test?

We have had one all along :-) See scylladb/scylladb#8203 - though they were in the context of Scylla, not the Python driver.
I didn't notice this PR until the two Scylla two unit tests for this bug which used to XFAIL (expected failure) magically started to pass :-) So I can confirm the fix indeed fixes this bug.

fruch pushed a commit that referenced this pull request Nov 21, 2021
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@ultrabug@haaawk@nyh@fruch