Skip to content

Integrate circuit breaker in AsyncTaskIndexService #73862

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 6 commits into from
Jun 9, 2021

Conversation

dnhatn
Copy link
Member

@dnhatn dnhatn commented Jun 7, 2021

This change integrates the circuit breaker in AsyncTaskIndexService to make sure that we won't hit OOM when serializing a large response of an async search.

The main change is in the second commit: 9ef8e2f.

Co-authored-by: Mayya Sharipova [email protected]

Related to #67594
Supersedes #73638

@dnhatn dnhatn added >enhancement :Search/Search Search-related issues that do not fall into other categories v8.0.0 v7.14.0 labels Jun 7, 2021
@elasticmachine elasticmachine added the Team:Search Meta label for search team label Jun 7, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search (Team:Search)

Copy link
Contributor

@jtibshirani jtibshirani left a comment

Choose a reason for hiding this comment

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

I left a couple ideas for simplifying the change. I was also wondering if we plan to address the read side too -- I think AsyncTaskIndexService#getEncodedResponse can currently consume several times the memory of the response since the document is first loaded, then parsed to a map, then the response is decoded/ deserialized.

@dnhatn
Copy link
Member Author

dnhatn commented Jun 7, 2021

I think AsyncTaskIndexService#getEncodedResponse can currently consume several times the memory of the response since the document is first loaded, then parsed to a map, then the response is decoded/ deserialized.

@jtibshirani We have a TODO for this. I will work on this in a follow up. Thank you for your review.

@dnhatn dnhatn requested a review from jtibshirani June 7, 2021 22:03
Copy link
Contributor

@jtibshirani jtibshirani left a comment

Choose a reason for hiding this comment

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

Thanks for clarifying. This looks good to me.

Copy link
Contributor

@mayya-sharipova mayya-sharipova left a comment

Choose a reason for hiding this comment

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

@dnhatn Thanks a lot! Great work! Everything LGTM, except an item about if we need also to close XContentBuilder in AsyncTaskIndexService.java

@dnhatn dnhatn merged commit b3d36d5 into elastic:master Jun 9, 2021
@dnhatn dnhatn deleted the circuit-breaker-async-search branch June 9, 2021 15:26
dnhatn added a commit to dnhatn/elasticsearch that referenced this pull request Jun 9, 2021
This change integrates the circuit breaker in AsyncTaskIndexService to
make sure that we won't hit OOM when serializing a large response of an
async search.

Related to elastic#67594
Supersedes  elastic#73638

Co-authored-by: Mayya Sharipova <[email protected]>
dnhatn added a commit that referenced this pull request Jun 10, 2021
This change integrates the circuit breaker in AsyncTaskIndexService to
make sure that we won't hit OOM when serializing a large response of an
async search.

Related to #67594
Supersedes  #73638

Co-authored-by: Mayya Sharipova <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>enhancement :Search/Search Search-related issues that do not fall into other categories Team:Search Meta label for search team v7.14.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants