Improve mechanism for extracting the result of a PlainActionFuture #110019
Conversation
server/src/main/java/org/elasticsearch/action/support/PlainActionFuture.java
Show resolved
Hide resolved
| return future.result(); | ||
| } catch (ExecutionException e) { | ||
| throw new UncategorizedExecutionException("An error occurred fetching timestamp field type for " + index, e); | ||
| } |
There was a problem hiding this comment.
This is not quite the same as before, I believe previously, if there was a RuntimeException it would be thrown directly, and if there was a checked exception the behaviour would be consistent with the above. But looking up the call stack there doesn't appear to be any behaviour specific to the exception thrown. I'm happy to try and restore the original behaviour but fear it might just be adding noise to be consistent with something that nobody was depending on in the first place.
There was a problem hiding this comment.
Yes I think this is ok, indeed possibly preferable, because now we get the call stack to the .result() call as well as the one which failed the future.
| throw e; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
I moved this down the hierarchy as it's used to prevent excessive nesting of execution exceptions here, but this one is private.
|
Hi @nicktindall, I've created a changelog YAML for you. |
|
Pinging @elastic/es-distributed (Team:Distributed) |
| } catch (Exception e) { | ||
| return fail(e); | ||
| } | ||
| } |
There was a problem hiding this comment.
This might be controversial. As the javadoc suggests it's just to reduce boilerplate in places where we don't want checked exceptions to propagate, and we don't expect them to occur. There's something like this in JUnit 5.
Perhaps there's already a utility for this?
There was a problem hiding this comment.
👍 I'd be inclined to call it safeGet since it's kinda doing the same thing as safeGet(Future<T>), getting a result and asserting that this succeeded.
| return future.result(); | ||
| } catch (ExecutionException e) { | ||
| throw new UncategorizedExecutionException("An error occurred fetching timestamp field type for " + index, e); | ||
| } |
There was a problem hiding this comment.
Yes I think this is ok, indeed possibly preferable, because now we get the call stack to the .result() call as well as the one which failed the future.
…_result # Conflicts: # test/framework/src/main/java/org/elasticsearch/test/ESTestCase.java
result()is called on aPlainActionFuturethat completed exceptionallynew ExecutionException(cause)PlainActionFuture#actionResult()as it makes less sense with the consistent wrappingCloses #108125