You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by "javanna (via GitHub)" <gi...@apache.org> on 2023/08/21 22:37:28 UTC
[GitHub] [lucene] javanna opened a new pull request, #12516: Unwrap execution exceptions cause and rethrow as is when possible
javanna opened a new pull request, #12516:
URL: https://github.com/apache/lucene/pull/12516
When performing concurrent search, we may get an execution exception from one or more slices. In that case, we'd like to rethrow the cause of the execution exception, which we do by wrapping it into a new runtime exception. Instead, we can rethrow runtime exceptions as-is, and the same is true for io exceptions. Any other exception is still wrapped into a new runtime exception. This unifies the exceptions that get thrown between sequential codepath (when no executor is provided) and concurrent codepath (when an executor is provided).
--
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@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] javanna merged pull request #12516: Unwrap execution exceptions cause and rethrow as is when possible
Posted by "javanna (via GitHub)" <gi...@apache.org>.
javanna merged PR #12516:
URL: https://github.com/apache/lucene/pull/12516
--
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@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] reta commented on a diff in pull request #12516: Unwrap execution exceptions cause and rethrow as is when possible
Posted by "reta (via GitHub)" <gi...@apache.org>.
reta commented on code in PR #12516:
URL: https://github.com/apache/lucene/pull/12516#discussion_r1300779063
##########
lucene/core/src/java/org/apache/lucene/search/TaskExecutor.java:
##########
@@ -57,6 +58,12 @@ final <T> List<T> invokeAll(Collection<RunnableFuture<T>> tasks) {
} catch (InterruptedException e) {
throw new ThreadInterruptedException(e);
} catch (ExecutionException e) {
+ if (e.getCause() instanceof IOException ioException) {
+ throw ioException;
+ }
+ if (e.getCause() instanceof RuntimeException runtimeException) {
+ throw runtimeException;
+ }
throw new RuntimeException(e.getCause());
Review Comment:
You may check for `Error` as well, to be safe:
```
if (e.getCause() instanceof Error error) {
throw error;
}
```
--
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@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] reta commented on a diff in pull request #12516: Unwrap execution exceptions cause and rethrow as is when possible
Posted by "reta (via GitHub)" <gi...@apache.org>.
reta commented on code in PR #12516:
URL: https://github.com/apache/lucene/pull/12516#discussion_r1300778596
##########
lucene/core/src/java/org/apache/lucene/search/TaskExecutor.java:
##########
@@ -57,6 +58,12 @@ final <T> List<T> invokeAll(Collection<RunnableFuture<T>> tasks) {
} catch (InterruptedException e) {
throw new ThreadInterruptedException(e);
} catch (ExecutionException e) {
+ if (e.getCause() instanceof IOException ioException) {
+ throw ioException;
Review Comment:
```suggestion
throw e.getCause();
```
##########
lucene/core/src/java/org/apache/lucene/search/TaskExecutor.java:
##########
@@ -57,6 +58,12 @@ final <T> List<T> invokeAll(Collection<RunnableFuture<T>> tasks) {
} catch (InterruptedException e) {
throw new ThreadInterruptedException(e);
} catch (ExecutionException e) {
+ if (e.getCause() instanceof IOException ioException) {
+ throw ioException;
Review Comment:
```suggestion
throw e.getCause();
```
--
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@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] javanna commented on a diff in pull request #12516: Unwrap execution exceptions cause and rethrow as is when possible
Posted by "javanna (via GitHub)" <gi...@apache.org>.
javanna commented on code in PR #12516:
URL: https://github.com/apache/lucene/pull/12516#discussion_r1315608401
##########
lucene/core/src/java/org/apache/lucene/search/AbstractKnnVectorQuery.java:
##########
@@ -95,20 +95,17 @@ public Query rewrite(IndexSearcher indexSearcher) throws IOException {
}
private TopDocs[] sequentialSearch(
- List<LeafReaderContext> leafReaderContexts, Weight filterWeight) {
- try {
- TopDocs[] perLeafResults = new TopDocs[leafReaderContexts.size()];
- for (LeafReaderContext ctx : leafReaderContexts) {
- perLeafResults[ctx.ord] = searchLeaf(ctx, filterWeight);
- }
- return perLeafResults;
- } catch (Exception e) {
- throw new RuntimeException(e);
Review Comment:
This looks like a way to compensate for the exception wrapping when doing concurrent search, by wrapping when doing sequential search do. This made it easier to test knn vector queries so far, regardless of whether rewrite is done concurrently or not. If we fix the root cause and remove the wrapping, we no longer need to artificially wrap here.
--
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@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] javanna commented on pull request #12516: Unwrap execution exceptions cause and rethrow as is when possible
Posted by "javanna (via GitHub)" <gi...@apache.org>.
javanna commented on PR #12516:
URL: https://github.com/apache/lucene/pull/12516#issuecomment-1687152027
Another one that you may be interested in @reta @sohami
--
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@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] javanna commented on a diff in pull request #12516: Unwrap execution exceptions cause and rethrow as is when possible
Posted by "javanna (via GitHub)" <gi...@apache.org>.
javanna commented on code in PR #12516:
URL: https://github.com/apache/lucene/pull/12516#discussion_r1315559947
##########
lucene/core/src/java/org/apache/lucene/search/TaskExecutor.java:
##########
@@ -57,6 +58,12 @@ final <T> List<T> invokeAll(Collection<RunnableFuture<T>> tasks) {
} catch (InterruptedException e) {
throw new ThreadInterruptedException(e);
} catch (ExecutionException e) {
+ if (e.getCause() instanceof IOException ioException) {
+ throw ioException;
+ }
+ if (e.getCause() instanceof RuntimeException runtimeException) {
+ throw runtimeException;
+ }
throw new RuntimeException(e.getCause());
Review Comment:
yep, thanks for the suggestion, makes sense to me.
--
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@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] javanna commented on a diff in pull request #12516: Unwrap execution exceptions cause and rethrow as is when possible
Posted by "javanna (via GitHub)" <gi...@apache.org>.
javanna commented on code in PR #12516:
URL: https://github.com/apache/lucene/pull/12516#discussion_r1315852459
##########
lucene/core/src/java/org/apache/lucene/search/TaskExecutor.java:
##########
@@ -57,6 +58,12 @@ final <T> List<T> invokeAll(Collection<RunnableFuture<T>> tasks) {
} catch (InterruptedException e) {
throw new ThreadInterruptedException(e);
} catch (ExecutionException e) {
+ if (e.getCause() instanceof IOException ioException) {
+ throw ioException;
+ }
+ if (e.getCause() instanceof RuntimeException runtimeException) {
+ throw runtimeException;
+ }
throw new RuntimeException(e.getCause());
Review Comment:
that's a really good idea, thanks, I did not know this method existed, and it's exactly what I needed.
--
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@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] jpountz commented on a diff in pull request #12516: Unwrap execution exceptions cause and rethrow as is when possible
Posted by "jpountz (via GitHub)" <gi...@apache.org>.
jpountz commented on code in PR #12516:
URL: https://github.com/apache/lucene/pull/12516#discussion_r1315641242
##########
lucene/core/src/java/org/apache/lucene/search/TaskExecutor.java:
##########
@@ -57,6 +58,12 @@ final <T> List<T> invokeAll(Collection<RunnableFuture<T>> tasks) {
} catch (InterruptedException e) {
throw new ThreadInterruptedException(e);
} catch (ExecutionException e) {
+ if (e.getCause() instanceof IOException ioException) {
+ throw ioException;
+ }
+ if (e.getCause() instanceof RuntimeException runtimeException) {
+ throw runtimeException;
+ }
throw new RuntimeException(e.getCause());
Review Comment:
Maybe reuse `IOUtils#rethrowAlways` here? `throw IOUtils.rethrowAlways(e.getCause());`
--
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@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org