HSEARCH-1353 + HSEARCH-1375 + HSEARCH-3110 Restore support for ErrorHandler (now FailureHandler) #2121
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
yrodiere
added
the
Waiting for other pull request
fax4ever
approved these changes
Oct 17, 2019
There was a problem hiding this comment.
I haven't found any significant issue and I think that we're doing amazing stuff.
I've only 3 comments over a nine-hours-long review!
IMHO, the exception handler, sorry the failure handler, is a very powerful tool now.
Great job and sorry again if I was very slow in reviewing it.
.../test/java/org/hibernate/search/documentation/reporting/failurehandler/MyFailureHandler.java
Outdated
Show resolved
Hide resolved
engine/src/main/java/org/hibernate/search/engine/reporting/IndexFailureContext.java
Show resolved
Hide resolved
yrodiere
removed
the
Waiting for other pull request
... and rename related concepts (ErrorContext, error_handler configuration property, ...). I used the term "Failure" because: 1. Exception isn't quite right, since we handle Throwables. 2. Error may incorrectly hint at java.lang.Error, and, again, we handle Throwables. 3. Failure has the advantage of being relatively exempt of a precise technical meaning.
We changed the APIs, these tags no longer make sense.
… to get the default implementation
…perations So it's really only used when there are index operations to mention. If a background thread of the mapper were to fail, for example during mass indexing, you would not get a call to handle(IndexFailureContext).
We're about to introduce more complexity with a type hierarchy, so we better start from something simple.
Pass a context no matter what, just a different type of context depending on the operation.
No need to make the FailureHandler API more complex just for this.
…Context.getUncommittedOperations() ... so that we can change "?" to a specific type later, without breaking backward compatibility.
…r upon Lucene indexing failures We used to only report works from the current workset after the one that failed, but really the uncommitted works before the failing work may also end up not being applied to the index, especially in case of commit failure. For example if we execute: 1. Workset 1: work 1, work 2 (commit strategy: no commit) 2. Workset 2: work 3, work 4, work 5 (commit strategy: no commit) ... and work 4 fails, then works 4 and 5 will obviously not be applied, but work 1, 2, and 3 may end up not being applied either, depending on the error, because we didn't commit them before the failure.
Currently it's not very important, since we would only have added the exception as a suppressed exception to itself (weird, but not critical). In the next commits, this will become an actual problem, because of how we handle errors differently.
…ailure handler upon Elasticsearch indexing failures 1. Report one failure per failed work. We used to report one failure with the first failed work as the "failing operation" and all other works as simply "uncommitted operations", but that's wrong: in some cases (bulk work failure) there are multiple failing works and the first one is not more responsible of the failure than the others. 2. Blame uncommitted works on the first failed work in each sequence. This is necessary now that we may report multiple failures per sequence.
…ause of its containing bulk work
It's no longer necessary, thanks to the changes in the last few commits.
…t code This method wraps exceptions with CompletionException, which is generally not what we want (we want the original exception).
…pletable futures or failure handlers 1. If we don't wrap the lambda passsed to exceptionally with Futures.handler, we end up keeping the CompletionException wrapper in the stack trace. That's really not necessary and makes us lose the original exception message when we extract it through e.getMessage(). 2. If we use expectRuntimeException instead of expectException, we end up losing the original exception message if we extract it through e.getMessage().
I want to add more features in there and the abstractions get in the way.
…h exceptions thrown in finally blocks
... on a best-effort basis, because several things are quite hard to assert.
yrodiere
force-pushed
the
HSEARCH-3110
branch
from
9f7d562
to
7cf49e0
Compare
…xpected exceptions It's rather critical in this case, since the failure handler is called in... failure handling code, where we usually don't expect another failure.
yrodiere
force-pushed
the
HSEARCH-3110
branch
from
7cf49e0
to
133f023
Compare
|
Addressed your comments and merged, thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Based on #2117 (HSEARCH-3736), which should be merged first.=> DoneThis moves the ErrorHandler to API, renames it to FailureHandler (see commit messages for an explanation), and brings a few changes aimed at improving the API, as well as removing a few bugs.
There are still a few issues which will be addressed in follow-up tickets; see the tickets linked from HSEARCH-1375.