Skip to content
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

SOLR-17677: Ensure DBQ is safe before running #3203

Conversation

gerlowskija
Copy link
Contributor

@gerlowskija gerlowskija commented Feb 20, 2025

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

Description

Several Solr Query implementations require a SolrIndexSearcher which is not always available. This often causes ClassCastExceptions and other errors when the queries are run by a non-SIS IndexSearcher, such as during delete-by-query operations.

Solution

This PR works around this limitation by detecting and blocking these queries during DBQs:

  1. A marker interface SolrSearcherRequirer (not thrilled with the name), is introduced that can by used by Query implementations that want to indicate their reliance on SolrIndexSearcher.
  2. A QueryVisitor implementation SolrSearcherRequirementDetector (again - could use help with naming) is introduced which uses Lucene's visit() support to determine if a part of a query requires a SIS.
  3. The DBQ code in DirectUpdateHandler2 uses this Visitor to throw a SolrException(400) if any unsupported queries are detected.

A better, longer-term fix would be to modify the implementation of these queries to not rely on SIS, but that approach would likely miss the upcoming 9.8.1 release. The solution above is sub-par, but at least gives us something in time for the 9.8.1 release.

Tests

New test cases in TestCloudDeleteByQuery. Unit tests in SolrSearcherRequirementDetectorTest.

Checklist

Please review the following and check all that apply:

  • I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
  • I have created a Jira issue and added the issue ID to my pull request title.
  • I have given Solr maintainers access to contribute to my PR branch. (optional but recommended, not available for branches on forks living under an organisation)
  • I have developed this patch against the main branch.
  • I have run ./gradlew check.
  • I have added tests for my changes.
  • I have added documentation for the Reference Guide

Portions of the DBQ codepath execute the deletion query with a standard
Lucene IndexSearcher, which upsets some Solr Query implementations that
have been built to assume SolrIndexSearcher in (e.g.)
Query.createWeight.

This commit adds checks for this case, which work by using a Lucene
"QueryVisitor" to iterate over a parsed query tree and detect whether
any of the individual queries require SolrIndexSearcher.
@github-actions github-actions bot added documentation Improvements or additions to documentation tests cat:search cat:cloud cat:index labels Feb 20, 2025
@gerlowskija
Copy link
Contributor Author

Check and tests pass for me locally.

The precommit PR check fails with the error below, but this is almost certainly unrelated to the PR. Will investigate a bit though to see if I can resolve...

/home/runner/work/solr/solr/solr/core/src/java/org/apache/solr/cloud/OverseerTaskProcessor.java:505: error: cannot find symbol
    } catch (AlreadyClosedException ignored) {
             ^
  symbol:   class AlreadyClosedException
  location: class OverseerTaskProcessor
> Task :solr:core:compileJava

@gerlowskija
Copy link
Contributor Author

Alright - looks like that was a problem on the 'main' I was based on - there was a revert on main and I've rebased on that now.

Should be good to go.

@gerlowskija gerlowskija merged commit 76e9b33 into apache:main Feb 22, 2025
2 of 3 checks passed
@gerlowskija gerlowskija deleted the SOLR-17677-ensure-query-is-dbq-safe-before-running branch February 22, 2025 02:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cat:cloud cat:index cat:search documentation Improvements or additions to documentation tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant