-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Handle errors in combine operator #9689
Handle errors in combine operator #9689
Conversation
Codecov Report
@@ Coverage Diff @@
## master #9689 +/- ##
=============================================
+ Coverage 28.13% 70.03% +41.90%
- Complexity 53 4932 +4879
=============================================
Files 1936 1948 +12
Lines 103927 104282 +355
Branches 15770 15809 +39
=============================================
+ Hits 29235 73032 +43797
+ Misses 71832 26129 -45703
- Partials 2860 5121 +2261
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
if (t instanceof Exception) { | ||
LOGGER.error("Caught exception while processing query: " + _queryContext, t); | ||
} else { | ||
LOGGER.error("Caught serious error while processing query: " + _queryContext, t); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if I follow the distinction here between an exception and a serious error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usually Java doesn't want user code to catch the Error
which indicates some serious problems (see Error.java
for more details). But for our specific case, we need to catch them in the sub-thread, or the thread will die without inserting the exceptions, and can cause unexpected behavior. Will add some comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a minor clarification on exception. LGTM otherwise
With the current combine algorithm, the errors must also be handled, or the query will either timeout or throw unexpected exception.