Skip to content

[Search] Remove client side shard timeout#75326

Closed
lizozom wants to merge 15 commits intoelastic:masterfrom
lizozom:search/remove-client-shard-timeout
Closed

[Search] Remove client side shard timeout#75326
lizozom wants to merge 15 commits intoelastic:masterfrom
lizozom:search/remove-client-shard-timeout

Conversation

@lizozom
Copy link
Copy Markdown
Contributor

@lizozom lizozom commented Aug 18, 2020 •
edited
Loading

Summary

Partially resolves #75321

Checklist

Remove client side shard timeout

For maintainers

@lizozom lizozom self-assigned this Aug 18, 2020
@lizozom lizozom mentioned this pull request Aug 18, 2020
Closed
@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
Copy Markdown
Contributor Author

lizozom commented Aug 18, 2020

@elasticmachine merge upstream

@lizozom
Copy link
Copy Markdown
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
Copy Markdown
Contributor

kibanamachine commented Aug 18, 2020 •
edited
Loading

💔 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

lukasolson
lukasolson reviewed Aug 18, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@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
Copy Markdown
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
Copy Markdown
Contributor

lukeelmers commented Aug 19, 2020

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

@lukeelmers lukeelmers mentioned this pull request Aug 26, 2020
Merged
3 tasks
@lizozom
Copy link
Copy Markdown
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

Reviewers

@lukasolson lukasolson lukasolson left review comments

Assignees

@lukeelmers lukeelmers

@lizozom lizozom

Labels

release_note:skip Skip the PR/issue when compiling release notes v7.10.0 v8.0.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Search] Update timeout configurations

5 participants

@lizozom @kibanamachine @lukeelmers @lukasolson @elasticmachine