-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
advanced setting to control search request preference #17271
Conversation
💔 Build Failed |
@gchaps Can you please provide feedback on the wording for |
💚 Build Succeeded |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool!
value: true, | ||
description: '<a href="https://www.elastic.co/guide/en/elasticsearch/reference/current/search-request-preference.html" target="_blank" rel="noopener noreferrer">Request Preference</a> ' + | ||
' controls the shard copies used for search execution.' + | ||
' Set to true to execute all search requests on the same shards. ' + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the description should mention the original reason we started using preference
, which I believe was caching (Please correct me if I'm wrong @rashidkpc).
Like your description states, not sending preference
sort of randomizes the node that handles the request, which means that the caches from a previous request might not be reusable on a second request and will need to be rebuilt.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about this
"Set to true to execute all search requests on the same shards. This has the benefit of reusing Shard Request Cache across requests. Set to false to have search request execution randomized among all available shard copies. Setting to false may provide better performance since requests can be spread across all shard copies but may result in inconsistent results as different shards may be in different refresh states."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder how many requests can actually even use the cache since... "Most queries that use now (see Date Mathedit) cannot be cached." from warning in shard-request-cache.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are other caches, like the caches from filters, which I'm almost positive are not impacted by using now
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
💚 Build Succeeded |
The way this is implemented right now might conflict with the zone awareness in Elasticsearch
If I set this setting to false, the performance might be much worse as I will only hit half of the cluster! A solution would be to set the preference to an empty string. This will then override the es default in case awareness is being used. Ideally a user would be able to choose between |
@LucaWintergerst Thanks for brining this up before the changes were committed. There is no reason why the setting can't be a list instead of a boolean. Just a couple of questions to make sure I understand everything.
|
|
after talking to a developer from the ES team we agreed that since this is an advanced setting it would be fine the way it is now. How much work would it be to make it customizable?
We need this distinction as |
I don't think we could do it with a single parameter but I could do this with 2 parameters.
How does that sound? |
that sounds great to me |
I'm thinking of something like this: Allows you to set which shards handle your search requests. I'm not sure how much you want to explain about customRequestPreference |
b301d65
to
08cba76
Compare
@spalger @stacey-gammon As discussed above, I have changed @gchaps I changed the wording of the session id to keep the bit about caching and added more details to |
💚 Build Succeeded |
@nreese I recommend breaking the last sentence into two sentences: This might provide better performance because requests can be spread across all shard copies. However, results might be inconsistent because different shards might be in different refresh states. We typically use "might" instead of "may" for localization reasons. |
💚 Build Succeeded |
@LucaWintergerst - The Preference documents say:
But if I am understanding correctly, that is only the default behavior when zone awareness is not set? When zone awareness is set, then the default behavior, when no preference is given, is @nreese - Looks great! Confirmed the requests look as expected and if I set it to something like |
@stacey-gammon it's not just |
* advanced setting to control search request preference * add header tests * add sentince about caching to description * change courier:setRequestPreference to list and add courier:customRequestPreference * update setting text
* advanced setting to control search request preference * add header tests * add sentince about caching to description * change courier:setRequestPreference to list and add courier:customRequestPreference * update setting text
fixes #15573
Provides advance setting
courier:setRequestPreference
. When true (default),perference
is set to sessionId as it always has been. When false,preference
is not set in the search header.