ResultSet: handle empty non-final pages on ResultSet iteration

[ultrabug]

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

[ultrabug]

I just added tests to the commit to demonstrate the problem and the fix.

[absurdfarce]

Nice test case... a very effective demonstration of the problem!

[absurdfarce]

I'm just a bit worried that this complicates next() a bit more than is necessary. The intermingling of self.next() and the global next(iter) functions here aren't as clear as one might like. Since our main goal is to ensure that we get a non-empty page out of fetch_next_page() why not test that explicitly?

        if not self.response_future._continuous_paging_session:
            # Some servers may return empty pages (Scylla is known to do so
            # in at least some cases) so make sure we have something non-empty
            # before we return.
            while True:
                self.fetch_next_page()
                if self._current_rows:
                    break
            self._page_iter = iter(self._current_rows)

Your code is effectively doing the same thing, it's just doing it by recursively calling self.next() to do so. I'm wondering if we can avoid the recursion entirely (and be clearer about our intent) by just handling the empty page case here.

What do you think?

[ultrabug]

Hi @absurdfarce

IMHO the right place to handle this is right where I've put it in the code logic because it makes it clearer that "in that case", we need to "replay the same logic of handling the iteration on pages".

As such, I feel the recursion is clean and less confusing than trying to be smart in a function which name has nothing to do with what needs to be done in the situation that we're covering here.

Once again, that's just my opinion of course and as you can see, that one line is all it takes ;)

[absurdfarce]

Apologies for the delay in getting back to you @ultrabug. I'm trying to juggle a number of things at once, ideally without dropping any of them on the floor. :)

I take your point about the minimal number of changes required to support your fix. I'm not 💯 sure I agree with doing it this way but I can certainly see the benefit of your approach. How about something of a compromise: perhaps you could add a simple comment in there noting that (a) empty pages are possible in some impls and (b) if the page we just fetched happens to be empty we'll do the right thing when we recurse? I think if that were there it would go a long way towards addressing my concern about clarity of the code.

[absurdfarce]

@ultrabug After thinking about this some more there's no reason to hold up accepting this PR for docs. What you have here is a good change and I certainly appreciate your work so far (including the aforementioned nice test!). I'll merge this in now and sort out the documentation question later.

[absurdfarce]

@ultrabug Thanks for the pull request! I had a question on the PR itself but I like what you have here, especially the nice unit test to clearly demonstrate the problem.

Have you signed the Contributor License Agreement for contributions to DataStax open source projects? If not you can find it at https://cla.datastax.com. Thanks!

[ultrabug]

@aboudreault

Have you signed the Contributor License Agreement for contributions to DataStax open source projects

I just did, yes. Thank you!

[ultrabug]

Thank you @absurdfarce , I see you added the comment as well 👍 sorry for the late reply