HSEARCH-3864 java.lang.Error thrown during mass indexing are swallowed and not reported to the user #2349
Conversation
| @@ -650,7 +610,7 @@ private SessionFactory setup() { | |||
| private enum ExecutionExpectation { | |||
| SUCCEED, | |||
| FAIL, | |||
| SKIP; | |||
| ERROR; | |||
There was a problem hiding this comment.
This is odd. I'd expect you would replace FAIL and maybe remove SKIP... but FAIL is apparently still used?
There was a problem hiding this comment.
This is the current behavior:
| AbstractMassIndexingErrorIT | AbstractMassIndexingFailureIT | |
|---|---|---|
| indexingAndFlush | FAIL | FAIL |
| indexingAndRefresh | FAIL | FAIL |
| flush | FAIL | FAIL |
| getId | ERROR | SKIP |
| purge | FAIL | FAIL |
| indexing | FAIL | FAIL |
| refresh | FAIL | FAIL |
| dropAndCreateSchema_exception | FAIL | FAIL |
| mergeSegmentsAfter | FAIL | FAIL |
| mergeSegmentsBefore | FAIL | FAIL |
| getTitle | ERROR | SKIP |
AbstractMassIndexingErrorIT behaves as AbstractMassIndexingFailureIT for all the cases with the exception of getId and getTitle.
That sounds reasonable to me. If it doesn't sound good for you, what would be the expected behavior?
| reset( failureHandler ); | ||
| failureHandler.handle( capture( entityFailureContextCapture ) ); | ||
| failureHandler.handle( capture( genericFailureContextCapture ) ); | ||
| failureHandler.handle( capture( genericFailureContextCapture ) ); |
There was a problem hiding this comment.
If you're going to use the capture like this, you should make it multi-valued to run assertions on all the captured values. i.e. use org.easymock.EasyMock#newCapture(org.easymock.CaptureType) to create the Capture object.
|
|
||
| import org.junit.Rule; | ||
|
|
||
| public class MassIndexingFailureCustomBackgroundErrorHandlerIT extends AbstractMassIndexingErrorIT { |
There was a problem hiding this comment.
The name of this class looks wrong. It tests behavior when errors occur during mass indexing, with a custom background failure handler. So it should be MassIndexingErrorCustomBackgroundFailureHandlerIT.
| import org.easymock.EasyMock; | ||
| import org.hamcrest.MatcherAssert; | ||
|
|
||
| public class MassIndexingFailureCustomMassIndexingErrorHandlerIT extends AbstractMassIndexingErrorIT { |
There was a problem hiding this comment.
The name of this class looks wrong. It tests behavior when errors occur during mass indexing, with a custom mass indexing failure handler. So it should be MassIndexingErrorCustomMassIndexingFailureHandlerIT.
|
|
||
| import org.apache.log4j.Level; | ||
|
|
||
| public class MassIndexingFailureDefaultBackgroundErrorHandlerIT extends AbstractMassIndexingErrorIT { |
There was a problem hiding this comment.
The name of this class looks wrong. It tests behavior when errors occur during mass indexing, with the default background failure handler. So it should be MassIndexingErrorDefaultBackgroundFailureHandlerIT.
fax4ever
force-pushed
the
HSEARCH-3864
branch
from
ccccc4c
to
1173bb0
Compare
fax4ever
force-pushed
the
HSEARCH-3864
branch
from
1173bb0
to
e551f62
Compare
Just propagate them.
...m/src/main/java/org/hibernate/search/mapper/orm/massindexing/MassIndexingFailureContext.java
Outdated
Show resolved
Hide resolved
...egrationtest/mapper/orm/massindexing/MassIndexingErrorDefaultBackgroundFailureHandlerIT.java
Outdated
Show resolved
Hide resolved
yrodiere
force-pushed
the
HSEARCH-3864
branch
from
701e075
to
162d74c
Compare
|
I added a few commits to simplify how Errors are reported: we will just propagate them and won't attempt to report them to the failure handlers. |
Just propagate the error as-is.
No wrapping, no calls to failure handlers, ... nothing. Just abort and propagate the error.
yrodiere
force-pushed
the
HSEARCH-3864
branch
from
162d74c
to
5b58919
Compare
|
Kudos, SonarCloud Quality Gate passed!
|
|
Merged, thanks! |
|
Thank you! |
https://hibernate.atlassian.net/browse/HSEARCH-3864
There is some code duplication.
I tried also to make [Make *MassIndexingFailureIT parametric] (fax4ever@0988a4e) as alternative...