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

ElasticsearchSource: Clear Scroll On Exhaustion or Completion #2330

Merged

Conversation

michaeljmarshall
Copy link
Contributor

References #2314

In the referenced issue, I had asked about the proper way to keep the stage alive for clean up when downstream has already finished. I noticed that some other stages were doing this by calling setKeepGoing(true), so I used that here. The documentation seems to support the use case, but please let me know if it looks incorrect.

I plan to add tests and documentation in the coming days. I wanted to get this PR up first to see if there was any feedback on the initial implementation.

@ennru
Copy link
Member

ennru commented May 29, 2020

Thank you for trying it out.
Your use of setKeepGoing is correct.
But I wonder if there is a good reason to do await the response from the delete request, we can't do anything but log it anyway. So alternatively this would be just a fire and forget and the stage can complete immediately.

@michaeljmarshall
Copy link
Contributor Author

My primary motivation for awaiting the response from the delete request is a clean shutdown. When I stop the stream early with a KillSwitch, I want to make sure that the source stage properly makes the delete call before letting the application shutdown. Given that there is a real cost to keeping a scroll open on the Elasticsearch side (and that the Elasticsearch documentation explicitly recommends clearing scrolls when they're no longer needed), it seems worth the slight delay to ensure that the call is made.

Regarding the logging, I do think it could be valuable for troubleshooting.

If you think awaiting the result of the delete request is too opinionated, I could add a setting to the ElasticsearchSourceSettings to allow end users to choose the right behavior for their use case.

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.

@ennru ennru added this to the 2.0.1 milestone Jun 1, 2020
@ennru ennru changed the title ElasticsearchSource: Clear Scroll On Scroll Exhaustion or Downstream Completion ElasticsearchSource: Clear Scroll On Exhaustion or Completion Jun 1, 2020
@ennru ennru merged commit ecc6472 into akka:master Jun 1, 2020
@ennru
Copy link
Member

ennru commented Jun 1, 2020

Thank you @michaeljmarshall, great to see Alpakka Elasticsearch being improved!

@michaeljmarshall michaeljmarshall deleted the elasticsearch-clear-scroll-feature branch June 4, 2020 04:35
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.

2 participants