Skip to content

JAVA-2934: Handle empty non-final pages in ReactiveResultSetSubscription #1544

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

Merged
merged 1 commit into from
Apr 28, 2021

Conversation

dssysolyatin
Copy link
Contributor

Hi !

I tried to use this driver with scylladb database, because it supports CQL protocol. But code started to hang for some queries:

val source = Source.fromPublisher(session.executeReactive(statement))
Await.result(source.runWith(Sink.ignore), scala.concurrent.duration.Duration.Inf))
println("finished")

The reason is that scylladb sometime returns a page with no data but with pagingState != null and it leads to code hanging

For instance, if pages queue contains three pages:

  • first page with data and with pagingState != null
  • second page without data and pagingState != null
  • third page without data and pagingState = null

Akka stream calls request(1) if it needs data and request calls drain that sends one row to akka stream subscriber. It works well for pages with data. But if there is page without data and pagingState != null the following situation appears:

Akka stream calls requests(1) that calls drain that calls tryNext. tryNext method detects that there is no data anymore in page 1 and it gets second page from pages queue. But because of second page has no data, tryNext() will return null and no data will be sent to akka stream subscriber. Because no data will be sent to akka subscriber, akka stream will not call request(1) anymore and drain will not be called from anywhere because all pages are already loaded.

I don't know why scylladb returns pages without data and pagingState != null. I will ask scylladb team, but it also would be good to fix this case in driver as well

@dssysolyatin dssysolyatin force-pushed the feature/scylladb-stuck-fix branch from 1e9767e to 8c2e369 Compare April 13, 2021 10:54
@dssysolyatin dssysolyatin changed the title Get rid of code hanging if there is empty page with next page attribute JAVA-2934: Get rid of code hanging if there is empty page with next page attribute Apr 13, 2021
@dssysolyatin dssysolyatin force-pushed the feature/scylladb-stuck-fix branch 2 times, most recently from 9892300 to c9b1c95 Compare April 13, 2021 14:57
Copy link
Contributor

@adutra adutra left a comment

Choose a reason for hiding this comment

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

Nice and elegant :-)

Could you please:

  • Add a unit test for this specific case in ReactiveResultSetSubscriptionTest
  • Change the commit and PR name to JAVA-2934: Handle empty non-final pages in ReactiveResultSetSubscription
  • Add an entry to the changelog:
4.12.0 (in progress)

- [bug] JAVA-2934: Handle empty non-final pages in ReactiveResultSetSubscription

Thanks!

@dssysolyatin dssysolyatin force-pushed the feature/scylladb-stuck-fix branch from c9b1c95 to 43471c0 Compare April 19, 2021 15:47
@dssysolyatin dssysolyatin changed the title JAVA-2934: Get rid of code hanging if there is empty page with next page attribute JAVA-2934: Handle empty non-final pages in ReactiveResultSetSubscription Apr 19, 2021
@dssysolyatin dssysolyatin requested a review from adutra April 19, 2021 15:48
@dssysolyatin dssysolyatin force-pushed the feature/scylladb-stuck-fix branch from 43471c0 to 24a3aac Compare April 19, 2021 16:02
@dssysolyatin
Copy link
Contributor Author

@adutra I fixed PR. Could you re-review my PR ?

Copy link
Contributor

@adutra adutra left a comment

Choose a reason for hiding this comment

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

Looking good, please do the changes I suggested and this should be good to go. Thank you for your time and involvement!

@dssysolyatin dssysolyatin force-pushed the feature/scylladb-stuck-fix branch from b481664 to 8eea388 Compare April 27, 2021 13:33
Copy link
Contributor

@adutra adutra left a comment

Choose a reason for hiding this comment

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

Thank you so much @dssysolyatin !

@dssysolyatin
Copy link
Contributor Author

Thank you for reviewing of PR @adutra

@adutra adutra merged commit b7e384e into apache:4.x Apr 28, 2021
@adutra adutra added this to the 4.11.1 milestone Apr 28, 2021
@adutra
Copy link
Contributor

adutra commented Apr 29, 2021

@dssysolyatin sorry I forgot the legal part :-) I need you to sign our CLA please:

https://cla.datastax.com/

Please ping me when you've signed it. Many thanks!

@dssysolyatin
Copy link
Contributor Author

@adutra No problem. Done ;)

ultrabug added a commit to scylladb/python-driver that referenced this pull request Aug 30, 2021
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
fruch pushed a commit to scylladb/python-driver that referenced this pull request Aug 30, 2021
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
absurdfarce pushed a commit to datastax/python-driver that referenced this pull request Sep 17, 2021
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
Sign up for free to 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.

2 participants