Skip to content

Fix SearchErrorTraceIT and friends to work with batched query execution #127150

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

original-brownbear
Copy link
Member

Making this work with batched execution and fixing a memory leak:

  • Fix memory leak by removing listener on first message. There really only is a single message here per node anyway with batched execution in the mix. Either it's a single shard on the data node and we get a single query message or it's multiple shards and we get a single batched message, so fine to remove listener after the first message since all tests do a single request only anyway.
  • Add a new hook that allows inspection of the actual response. This is needed for batched since batched sends a non-error response even if the data node failed all searches. We had this before in the onResponseSent hook but checking the instance after it's been sent over the wire causes needless overhead in the production code so moving to a "before-style" hook here.

part of #125788

Making this work with batched execution and fixing a memory leak:

* Fix memory leak by removing listener on first message. There really only is a single message
here per node anyway with batched execution in the mix. Either it's a single shard on the data node
and we get a single query message or it's multiple shards and we get a single batched message,
so fine to remove listener after the first message since all tests do a single request only anyway.
* Add a new hook that allows inspection of the actual response. This is needed for batched since batched
sends a non-error response even if the data node failed all searches. We had this before in the
`onResponseSent` hook but checking the instance after it's been sent over the wire causes needless
overhead in the production code so moving to a "before-style" hook here.
@original-brownbear original-brownbear added >test Issues or PRs that are addressing/adding tests :Search Foundations/Search Catch all for Search Foundations labels Apr 22, 2025
@elasticsearchmachine elasticsearchmachine added Team:Search Foundations Meta label for the Search Foundations team in Elasticsearch v9.1.0 labels Apr 22, 2025
@elasticsearchmachine
Copy link
Collaborator

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

* @param action the request action
* @param response response instance
*/
default void onBeforeResponseSent(long requestId, String action, TransportResponse response) {}
Copy link
Member Author

Choose a reason for hiding this comment

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

@DaveCTurner wdyt? this should be ok right? In prod the overhead is negligible I think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Search Foundations/Search Catch all for Search Foundations Team:Search Foundations Meta label for the Search Foundations team in Elasticsearch >test Issues or PRs that are addressing/adding tests v9.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants