You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by "dsmiley (via GitHub)" <gi...@apache.org> on 2023/03/11 07:42:31 UTC
[GitHub] [solr] dsmiley opened a new pull request, #1450: SOLR-16693: timeAllowed: use more efficient TimeLimitingBulkScorer
dsmiley opened a new pull request, #1450:
URL: https://github.com/apache/solr/pull/1450
Instead of ExitableDirectoryReader
https://issues.apache.org/jira/browse/SOLR-16693
TODO... maybe allow use of ExitableDirectoryReader via sys prop? BTW there are code paths using the reader that wouldn't use the search method yet may have a timeAllowed param. I think I've seen this in an erroneous (bug or accident; haven't root caused yet) scenario at work but could be valid.
Wish we had benchmarks on this. Any way, it's interesting that all tests pass so easily :-)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on pull request #1450: SOLR-16693: timeAllowed: use more efficient TimeLimitingBulkScorer
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #1450:
URL: https://github.com/apache/solr/pull/1450#issuecomment-1471936161
BTW I won't merge this any sooner than the Lucene 9.5 PR merges, so that these both can ultimately ship on the same release.
Proposed CHANGES.txt under improvements:
* SOLR-16693: For query timeAllowed, switch from ExitableDirectoryReader to TimeLimitingBulkScorer
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley merged pull request #1450: SOLR-16693: timeAllowed: use more efficient TimeLimitingBulkScorer
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley merged PR #1450:
URL: https://github.com/apache/solr/pull/1450
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] HoustonPutman commented on a diff in pull request #1450: SOLR-16693: timeAllowed: use more efficient TimeLimitingBulkScorer
Posted by "HoustonPutman (via GitHub)" <gi...@apache.org>.
HoustonPutman commented on code in PR #1450:
URL: https://github.com/apache/solr/pull/1450#discussion_r1156086732
##########
solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java:
##########
@@ -716,23 +723,42 @@ public QueryResult search(QueryResult qr, QueryCommand cmd) throws IOException {
return qr;
}
- // FIXME: This option has been dead/noop since 3.1, should we re-enable or remove it?
- // public Hits search(Query query, Filter filter, Sort sort) throws IOException {
- // // todo - when Solr starts accepting filters, need to
- // // change this conditional check (filter!=null) and create a new filter
- // // that ANDs them together if it already exists.
- //
- // if (optimizer==null || filter!=null || !(query instanceof BooleanQuery)
- // ) {
- // return super.search(query,filter,sort);
- // } else {
- // Query[] newQuery = new Query[1];
- // Filter[] newFilter = new Filter[1];
- // optimizer.optimize((BooleanQuery)query, this, 0, newQuery, newFilter);
- //
- // return super.search(newQuery[0], newFilter[0], sort);
- // }
- // }
+ @Override
+ protected void search(List<LeafReaderContext> leaves, Weight weight, Collector collector)
+ throws IOException {
+ final var queryTimeout = SolrQueryTimeoutImpl.getInstance();
+ if (useExitableDirectoryReader || queryTimeout.isTimeoutEnabled() == false) {
+ // no timeout. Pass through to super class
+ super.search(leaves, weight, collector);
+ } else {
+ // Timeout enabled! This impl is maybe a hack. Use Lucene's IndexSearcher timeout.
+ // But only some queries have it so don't use on "this" (SolrIndexSearcher), not to mention
+ // that timedOut() might race with concurrent queries (dubious design).
+ // So we need to make a new IndexSearcher instead of using "this".
+ new IndexSearcher(reader) { // cheap, actually!
Review Comment:
Maybe we should open up a Lucene issue that gives us a search(...., timeout) method, so that this hack isn't necessary in the future. Though as you mention, that would include adding a similar TimeAllowedExceeded Exception to Lucene.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] cpoerschke commented on a diff in pull request #1450: SOLR-16693: timeAllowed: use more efficient TimeLimitingBulkScorer
Posted by "cpoerschke (via GitHub)" <gi...@apache.org>.
cpoerschke commented on code in PR #1450:
URL: https://github.com/apache/solr/pull/1450#discussion_r1136792660
##########
solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java:
##########
@@ -713,6 +713,33 @@ public QueryResult search(QueryResult qr, QueryCommand cmd) throws IOException {
return qr;
}
+ @Override
+ protected void search(List<LeafReaderContext> leaves, Weight weight, Collector collector)
+ throws IOException {
+ final var queryTimeout = SolrQueryTimeoutImpl.getInstance();
+ if (queryTimeout.isTimeoutEnabled() == false) {
+ // no timeout. Pass through to subclass
Review Comment:
```suggestion
// no timeout. Pass through to super class
```
##########
solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java:
##########
@@ -713,6 +713,33 @@ public QueryResult search(QueryResult qr, QueryCommand cmd) throws IOException {
return qr;
}
+ @Override
+ protected void search(List<LeafReaderContext> leaves, Weight weight, Collector collector)
+ throws IOException {
+ final var queryTimeout = SolrQueryTimeoutImpl.getInstance();
+ if (queryTimeout.isTimeoutEnabled() == false) {
+ // no timeout. Pass through to subclass
+ super.search(leaves, weight, collector);
+ } else {
+ // Timeout enabled! This impl is maybe a hack. Use Lucene's IndexSearcher timeout.
+ // But only some queries have it so don't use on "this" (SolrIndexSearcher), not to mention
+ // that timedOut() might race with concurrent queries (dubious design).
+ // So we need to make a new IndexSearcher instead of using "this".
+ new IndexSearcher(reader) { // cheap, actually!
+ void searchWithTimeout() throws IOException {
+ setTimeout(queryTimeout.makeLocalImpl());
+ super.search(leaves, weight, collector); // FYI protected access
+ if (timedOut()) {
+ // We might not even be using ExitableDirectoryReader yet various places in Solr check
+ // for this exception. Sometimes others but this seems most suitable.
+ throw new ExitableDirectoryReader.ExitingReaderException(
+ "Timed out. Not actually via ExitableDirectoryReader");
Review Comment:
Maybe an alternative might be to have a `class SolrNotActuallyExitableDirectoryReader extends ExitableDirectoryReader.ExitingReaderException` approach which would be thrown here and then over time the various places could be switched over to check for `SolrNotActuallyExitableDirectoryReader` instead or to have the checks phased out.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a diff in pull request #1450: SOLR-16693: timeAllowed: use more efficient TimeLimitingBulkScorer
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #1450:
URL: https://github.com/apache/solr/pull/1450#discussion_r1137585507
##########
solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java:
##########
@@ -713,6 +713,33 @@ public QueryResult search(QueryResult qr, QueryCommand cmd) throws IOException {
return qr;
}
+ @Override
+ protected void search(List<LeafReaderContext> leaves, Weight weight, Collector collector)
+ throws IOException {
+ final var queryTimeout = SolrQueryTimeoutImpl.getInstance();
+ if (queryTimeout.isTimeoutEnabled() == false) {
+ // no timeout. Pass through to subclass
+ super.search(leaves, weight, collector);
+ } else {
+ // Timeout enabled! This impl is maybe a hack. Use Lucene's IndexSearcher timeout.
+ // But only some queries have it so don't use on "this" (SolrIndexSearcher), not to mention
+ // that timedOut() might race with concurrent queries (dubious design).
+ // So we need to make a new IndexSearcher instead of using "this".
+ new IndexSearcher(reader) { // cheap, actually!
+ void searchWithTimeout() throws IOException {
+ setTimeout(queryTimeout.makeLocalImpl());
+ super.search(leaves, weight, collector); // FYI protected access
+ if (timedOut()) {
+ // We might not even be using ExitableDirectoryReader yet various places in Solr check
+ // for this exception. Sometimes others but this seems most suitable.
+ throw new ExitableDirectoryReader.ExitingReaderException(
+ "Timed out. Not actually via ExitableDirectoryReader");
Review Comment:
I assume you're not serious about the name SolrNotActuallyExitableDirectoryReader. Two name proposals:
* TimeAllowedExceededException -- generic. But despite it being generic, the reality is that there are other exceptions that are effectively similar that would still exist, so maybe give up on trying to be generic?
* TimeAllowedExceededFromScorerException -- more specific as to what makes this timeout what it is.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a diff in pull request #1450: SOLR-16693: timeAllowed: use more efficient TimeLimitingBulkScorer
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #1450:
URL: https://github.com/apache/solr/pull/1450#discussion_r1138646932
##########
solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java:
##########
@@ -713,6 +713,33 @@ public QueryResult search(QueryResult qr, QueryCommand cmd) throws IOException {
return qr;
}
+ @Override
+ protected void search(List<LeafReaderContext> leaves, Weight weight, Collector collector)
+ throws IOException {
+ final var queryTimeout = SolrQueryTimeoutImpl.getInstance();
+ if (queryTimeout.isTimeoutEnabled() == false) {
+ // no timeout. Pass through to subclass
+ super.search(leaves, weight, collector);
+ } else {
+ // Timeout enabled! This impl is maybe a hack. Use Lucene's IndexSearcher timeout.
+ // But only some queries have it so don't use on "this" (SolrIndexSearcher), not to mention
+ // that timedOut() might race with concurrent queries (dubious design).
+ // So we need to make a new IndexSearcher instead of using "this".
+ new IndexSearcher(reader) { // cheap, actually!
+ void searchWithTimeout() throws IOException {
+ setTimeout(queryTimeout.makeLocalImpl());
+ super.search(leaves, weight, collector); // FYI protected access
+ if (timedOut()) {
+ // We might not even be using ExitableDirectoryReader yet various places in Solr check
+ // for this exception. Sometimes others but this seems most suitable.
+ throw new ExitableDirectoryReader.ExitingReaderException(
+ "Timed out. Not actually via ExitableDirectoryReader");
Review Comment:
Good; I chose the latter; PR updated.
I think it'd be worthwhile for Lucene to have a base class of these exceptions that is public, therefore Lucene apps can catch one general exception no matter which approach is being taken.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on pull request #1450: SOLR-16693: timeAllowed: use more efficient TimeLimitingBulkScorer
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #1450:
URL: https://github.com/apache/solr/pull/1450#issuecomment-1493135072
I plan to merge end of Monday or so.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] cpoerschke commented on a diff in pull request #1450: SOLR-16693: timeAllowed: use more efficient TimeLimitingBulkScorer
Posted by "cpoerschke (via GitHub)" <gi...@apache.org>.
cpoerschke commented on code in PR #1450:
URL: https://github.com/apache/solr/pull/1450#discussion_r1138283647
##########
solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java:
##########
@@ -713,6 +713,33 @@ public QueryResult search(QueryResult qr, QueryCommand cmd) throws IOException {
return qr;
}
+ @Override
+ protected void search(List<LeafReaderContext> leaves, Weight weight, Collector collector)
+ throws IOException {
+ final var queryTimeout = SolrQueryTimeoutImpl.getInstance();
+ if (queryTimeout.isTimeoutEnabled() == false) {
+ // no timeout. Pass through to subclass
+ super.search(leaves, weight, collector);
+ } else {
+ // Timeout enabled! This impl is maybe a hack. Use Lucene's IndexSearcher timeout.
+ // But only some queries have it so don't use on "this" (SolrIndexSearcher), not to mention
+ // that timedOut() might race with concurrent queries (dubious design).
+ // So we need to make a new IndexSearcher instead of using "this".
+ new IndexSearcher(reader) { // cheap, actually!
+ void searchWithTimeout() throws IOException {
+ setTimeout(queryTimeout.makeLocalImpl());
+ super.search(leaves, weight, collector); // FYI protected access
+ if (timedOut()) {
+ // We might not even be using ExitableDirectoryReader yet various places in Solr check
+ // for this exception. Sometimes others but this seems most suitable.
+ throw new ExitableDirectoryReader.ExitingReaderException(
+ "Timed out. Not actually via ExitableDirectoryReader");
Review Comment:
> I assume you're not serious about the name SolrNotActuallyExitableDirectoryReader. Two name proposals: ...
Yeah, that was not meant to be a serious name suggestion but only a way to convey the `class ... extends ExitableDirectoryReader.ExitingReaderException` idea.
`TimeAllowedExceededException` vs. `TimeAllowedExceededFromScorerException` -- the latter to me seems closer to the current `ExitableDirectoryReader.ExitingReaderException` which is also pretty specific.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org