SAI-4737: System prop solr.MaxBasicQueriesOverride for surround query
#172
Conversation
…log it once. And solr.absoluteMaxBasicQueries should only kick in if the original maxBasicQueries is greater
| @@ -77,6 +106,16 @@ public Query parse() throws SyntaxError { | |||
| this.maxBasicQueries = DEFMAXBASICQUERIES; | |||
| } | |||
| } | |||
|
|
|||
| if (ABSOLUTE_MAX_BASIC_QUERIES > 0 && this.maxBasicQueries > ABSOLUTE_MAX_BASIC_QUERIES) { | |||
There was a problem hiding this comment.
How about just setting value, if that is positive. Probably we don't need to log here.
There was a problem hiding this comment.
This current logic basically only considers "solr.absoluteMaxBasicQueries" if it was provided and positive. Pretty similar to what your are suggesting?
Are you suggesting "solr.absoluteMaxBasicQueries" will just override maxBasicQueries in all cases? Currently "solr.absoluteMaxBasicQueries" acts as a absolute safeguard, maxBasicQueries is still effective as far as it stays below "solr.absoluteMaxBasicQueries".
The log was mostly to avoid the case that user is confused which a higher maxBasicQueries seems to have no impact (because of the solr.absoluteMaxBasicQueries). I have some concern that this could be noisy, though once per surround query is probably not too bad imo :) ?
There was a problem hiding this comment.
log would be per query, per shard ?
There was a problem hiding this comment.
It was per query on core I believe.
Since now it's a temporary override (the dev should be well aware of it, unlike the proposal before which could be long living), I have removed the logging :)
… this requires less deployment but is a more temporary implementation.
…ry-system-prop' into patsonluk/SAI-4737-max-basic-query-system-prop # Conflicts: # solr/core/src/java/org/apache/solr/search/SurroundQParserPlugin.java
patsonluk
changed the title
solr.absoluteMaxBasicQueries for surround querysolr.MaxBasicQueriesOverride for surround query
|
|
||
| if (MAX_BASIC_QUERIES_OVERRIDE > 0) { | ||
| this.maxBasicQueries = MAX_BASIC_QUERIES_OVERRIDE; | ||
| } | ||
|
|
There was a problem hiding this comment.
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.
@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:
- Set absolute max override for all nodes as 10k, except selected nodes with 50k (and higher solr.max.booleanClauses)
- Redeploy all node so all nodes so they can process the absolute max
- Set higher
maxBasicQueriesfrom client side
While with the current approach:
- Set higher
solr.maxBasicQueriesOverride(and higher solr.max.booleanClauses) only on selected nodes - Redeploy on only selected nodes so they can process the absolute max
- All requests coming into the selected nodes will use higher limits (on all orgs). There is no need to set higher
maxBasicQueriesfrom 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.
🤦 of course! sorry I overlooked the discussion that resulted in going this route, and thanks for the explanation.
…ry (#172) * Added support of system prop solr.absoluteMaxBasicQueries for surround query * Moved param lookup logic to static, as we should only need to do and log it once. And solr.absoluteMaxBasicQueries should only kick in if the original maxBasicQueries is greater * ./gradlew tidy * Changed solr.absoluteMaxBasicQueries to solr.maxBasicQueriesOverride, this requires less deployment but is a more temporary implementation. * Renamed variable * ./gradlew tidy
Description
To add a System prop
solr.maxBasicQueriesOverridewhich:solr.maxBasicQueriesOverridevalue will always override the parammaxBasicQueriesfrom surround query.Solution
Modified
SurroundQParserPluginto read the system property (this happens only once per class).Take note that this is meant to be a temporary property for testing. We should NOT have this value set in long run, it should respect the
maxBasicQueriesfrom query param eventually.Tests
Tested locally with case of 11000 matches surround query:
maxBasicQuery=10000and nosolr.maxBasicQueriesOverride-> TooManyBasicQueries errormaxBasicQuery=10000and set system propertysolr.maxBasicQueriesOverride=15000-> No errorTake note that this is run on unit test case
MiniSolrCloudClusterwithIndexSearcher.setMaxClauseCount(50_000). Otherwise the case would fail withorg.apache.solr.common.SolrException: org.apache.lucene.search.IndexSearcher$TooManyNestedClauses: Query contains too many nested clauses; maxClauseCount is set to 1024Remarks
This change will apply the override on all orgs, so we cannot do per org control with this change. However, that's probably okay to do it per node only.