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

[Search] Remove client side shard timeout #75326

Closed

Conversation

lizozom
Copy link
Contributor

@lizozom lizozom commented Aug 18, 2020

Summary

Partially resolves #75321

Checklist

Remove client side shard timeout

For maintainers

@lizozom lizozom self-assigned this Aug 18, 2020
@lizozom lizozom added release_note:skip Skip the PR/issue when compiling release notes Team:AppArch v8.0.0 labels Aug 18, 2020
@lizozom
Copy link
Contributor Author

lizozom commented Aug 18, 2020

@elasticmachine merge upstream

@lizozom
Copy link
Contributor Author

lizozom commented Aug 18, 2020

@elasticmachine merge upstream

@lizozom lizozom requested a review from lukasolson August 18, 2020 18:22
@lizozom lizozom changed the title [Search] Remove client shard timeout [Search] Remove client side shard timeout Aug 18, 2020
@kibanamachine
Copy link
Contributor

kibanamachine commented Aug 18, 2020

💔 Build Failed

Failed CI Steps

Build metrics

page load bundle size

id value diff baseline
data 1.4MB -873.0B 1.4MB
visTypeVega 661.7KB -70.0B 661.8KB
total -943.0B

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

Copy link
Member

@lukasolson lukasolson left a comment

Choose a reason for hiding this comment

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

Hmm... Since msearch doesn't follow the normal pattern of going through our server-side search strategies, then if it's removed here, then how do we ensure that the parameter is sent in our msearch requests? We could possibly add it in the create_proxy file and import our existing server-side getSearchParams in the new platform. Thoughts?

Also, I think we can get rid of a couple of additional parameters that are sent, including ignore_unavailable, rest_total_hits_as_int, and even ignore_throttled since they are already configured server-side. We could also move max_concurrent_shard_requests to the server, but that might be outside the scope of this PR.

@lizozom
Copy link
Contributor Author

lizozom commented Aug 19, 2020

@lukeelmers Since msearch uses a uniquely formatted body, it seems to be impossible injecting the timeout on the server or any other parameter for the time being, without parsing and re-creating the body string.

Once we create our msearch route, we could simplify the way the data is sent to that endpoint (I would use a JSON array, with each object having a body and a header option). Then on the server we will be able to format them correctly and inject the desired parameters.

@lukeelmers
Copy link
Member

Okay @lizozom, so it sounds like it's best to revisit after #55139 is resolved

@lizozom
Copy link
Contributor Author

lizozom commented Aug 27, 2020

Addressed in #75943 and #75728

@lizozom lizozom closed this Aug 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes v7.10.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Search] Update timeout configurations
5 participants