Skip to content

SOLR-17831: Fix the bug in QueryLimits initialization in SolrIndexSearcher.#3446

Merged
sigram merged 7 commits intoapache:mainfrom
sigram:jira/solr-17831
Aug 1, 2025
Merged

SOLR-17831: Fix the bug in QueryLimits initialization in SolrIndexSearcher.#3446
sigram merged 7 commits intoapache:mainfrom
sigram:jira/solr-17831

Conversation

@sigram
Copy link
Contributor

@sigram sigram commented Jul 29, 2025

Improve the helper class to better track locations and counts where limits are enforced.

https://issues.apache.org/jira/browse/SOLR-17831

This PR will also improve testability of using ExitableDirectoryReader by better instrumentation in CallerSpecificQueryLimit and making the sysproperty modifiable by using EnvUtils.

Improve the helper class to better track locations and counts where limits are enforced.
sigram added 2 commits July 30, 2025 13:57
Create a unit test to verify the use of ExitableDirectoryReader.
@sigram sigram marked this pull request as ready for review July 30, 2025 12:13
Copy link
Contributor

@dsmiley dsmiley left a comment

Choose a reason for hiding this comment

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

that's some impressive testing... I didn't know we had the ability to assert who exactly tripped the limit.

throws IOException {
QueryLimits queryLimits = QueryLimits.getCurrentLimits();
if (useExitableDirectoryReader || !queryLimits.isLimitsEnabled()) {
if (EnvUtils.getPropertyAsBool(EXITABLE_READER_PROPERTY) || !queryLimits.isLimitsEnabled()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

should this instead check if the existing reader is an instance of ExitableDirectoryReader? It would have already been wrapped.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wonder in what scenario this would happen, realistically ... and whether we should care here. We need to make sure that the way we wrapped the original reader uses our implementation of the QueryTimeout. We have no way to determine that if the input is an EDR of unknown origin.

@sigram
Copy link
Contributor Author

sigram commented Jul 31, 2025

The helper class CallerSpecificQueryLimit helped me to preserve my sanity while working on SOLR-17172, when trying to figure out why a limit was reached (or the opposite, as was often the case). The way it works is not really specific to the QueryLimits api, so if you think it could be useful for some other tests then it's easy to generalize it.

@sigram sigram merged commit 199eed9 into apache:main Aug 1, 2025
3 checks passed
sigram added a commit to sigram/solr that referenced this pull request Aug 1, 2025
sigram added a commit to sigram/solr that referenced this pull request Aug 1, 2025
sigram added a commit that referenced this pull request Aug 1, 2025
sigram added a commit that referenced this pull request Aug 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants