Skip to content
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

Cassandra: check isFullyFetched before completion #1935

Merged
merged 1 commit into from
Oct 15, 2019

Conversation

anzecesar
Copy link
Contributor

Purpose

Under some conditions, the source completed before all results were loaded from the result set.

Now it only completes when rs is fully fetched and all elements were emitted. Before it was completing in case where the current fetched buffer was empty.

References

References #1934

Changes

  • only completes the stream when rs.isFullyFetched
  • if just the currently loaded set is empty, it fetches more (an edge case)

Background Context

I observed this issue where fetchSize was set relatively low and the
downstream consumer was fast.

…rom the

result set.

I observed this issue where fetchSize was set relatively low and the
downstream consumer was fast.
@ennru ennru changed the title Fixes a case where the stage completes before all results were read Cassandra: check isFullyFetched before completion Sep 17, 2019
Copy link
Member

@ennru ennru left a comment

Choose a reason for hiding this comment

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

Can you come up with a test case that shows the broken and now fixed behaviour?

@anzecesar
Copy link
Contributor Author

@ennru I can try, but this was already hard to reproduce. Are there any existing tests with cassandra already?

@ennru
Copy link
Member

ennru commented Sep 17, 2019

Yes, they use Cassandra via Docker compose.

@anzecesar
Copy link
Contributor Author

@ennru Where can I find them? There's nothing in https://github.com/akka/alpakka/tree/master/cassandra/src/test

@anzecesar
Copy link
Contributor Author

@ennru Tnx. For some reason I assumed the purpose of that was more or less just for the docs and haven't even considered it a working spec 🤷‍♂️. Well, we all know where assumption leads 😂.

I'll try to add a test case for this although I suspect it will be at best very flaky.

@ennru ennru requested a review from chbatey October 2, 2019 11:56
@anzecesar
Copy link
Contributor Author

I finally got some time to work on this today and I was not able to reproduce this locally with a spec. The conditions under which I originally encountered the issue were reading ~10k rows containing quite large jsons (around 150 mb for the whole dataset). I'm not exactly sure what exactly led to the buffer being exhausted prematurely.

There is however an exiting test, that generally ensures this doesn't break any functionality:

"stream the result of a Cassandra statement with several pages" in {

Copy link
Member

@ennru ennru left a comment

Choose a reason for hiding this comment

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

LGTM.

// fetch next page
val gFut = rs.fetchMoreResults()
val exec = materializer.executionContext
GuavaFutures.invokeTryCallback(gFut, exec)(futFetchedCallback)
Copy link
Member

Choose a reason for hiding this comment

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

should this be using an async call back?

Copy link
Member

Choose a reason for hiding this comment

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

It looks OK to me, it is only called within synchronous code.
tryPushAfterFetch is called via the async handler futFetchedCallback when the future returns.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I just refactored it out into a method to not duplicate code.

@ennru ennru merged commit 264cc44 into akka:master Oct 15, 2019
@ennru ennru added this to the 1.1.3 milestone Oct 15, 2019
@ennru
Copy link
Member

ennru commented Oct 15, 2019

Thank you for this fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants