Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
SAI-4737: System prop
solr.MaxBasicQueriesOverride
for surround query #172SAI-4737: System prop
solr.MaxBasicQueriesOverride
for surround query #172Changes from all commits
748787a
43bde54
27232ba
cd19bc0
3495cdc
c0ceec7
4102f3c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Is it preferable to have the sysprop completely override user-supplied localParam values? My understanding is that we want this to be an absolute ceiling on configured vals -- not to be an absolute override. i.e., query should still be able to specify a lower mbq setting, right?
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.
@magibney Thank you for the question!
Yes that was the previous implementation (absolute max). But as discussed with @hiteshk25 , the previous implementation would have required re-deployment on all nodes since with such approach we would need to:
maxBasicQueries
from client sideWhile with the current approach:
solr.maxBasicQueriesOverride
(and higher solr.max.booleanClauses) only on selected nodesmaxBasicQueries
from client side.So the current approach has major pro of not needing to restart all nodes (and the code logic is slightly simpler). The con is that we can no longer control it per org and such override basically just ignore all
maxBasicQueries
param from surround query.The approach now meant to be a temporary measure for testing, I don't think we want to keep this around at all. We might even want to revert this commit later on since such system prop does not make sense in any normal scenario (unlike the absolute max, which could be useful in some normal scenarios)
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.
🤦 of course! sorry I overlooked the discussion that resulted in going this route, and thanks for the explanation.