ctubbsii commented on a change in pull request #1840:
URL: https://github.com/apache/accumulo/pull/1840#discussion_r545345733
##########
File path:
server/tserver/src/main/java/org/apache/accumulo/tserver/scan/LookupTask.java
##########
@@ -177,9 +177,13 @@ public void run() {
}
} catch (SampleNotPresentException e) {
addResult(e);
- } catch (Throwable e) {
+ } catch (Exception e) {
log.warn("exception while doing multi-scan ", e);
addResult(e);
+ } catch (Error t) {
+ log.warn("Error while doing multi-scan ", t);
+ addResult(t);
Review comment:
Is there value in adding this to the result if we're re-throwing?
##########
File path:
server/tserver/src/main/java/org/apache/accumulo/tserver/scan/NextBatchTask.java
##########
@@ -89,14 +88,16 @@ public void run() {
}
} catch (TooManyFilesException | SampleNotPresentException e) {
addResult(e);
- } catch (OutOfMemoryError ome) {
- Halt.halt("Ran out of memory scanning " + scanSession.extent + " for " +
scanSession.client,
- 1);
- addResult(ome);
- } catch (Throwable e) {
+ } catch (Exception e) {
log.warn("exception while scanning tablet "
+ (scanSession == null ? "(unknown)" : scanSession.extent), e);
addResult(e);
+ } catch (Error t) {
+ log.warn(
+ "Error while scanning tablet " + (scanSession == null ? "(unknown)"
: scanSession.extent),
+ t);
Review comment:
The message here has changed and no longer has the scan session client
included. Also, you should use slf4j substitution syntax to format the message.
Are we sure we want to change the behavior to no longer Halt here? Is this
change dependent on the one in #1818 to handle the shutdown?
##########
File path:
server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/MinorCompactionTask.java
##########
@@ -111,7 +111,7 @@ public void run() {
if (tablet.needsSplit()) {
tablet.getTabletServer().executeSplit(tablet);
}
- } catch (Throwable t) {
+ } catch (Exception t) {
Review comment:
This seems to be wrapping the exception with a RTE. Are there even
checked exceptions that can occur here? If so, we probably should enumerate
them explicitly, and only wrap checked exceptions, leaving RTEs unwrapped.
##########
File path:
server/tserver/src/main/java/org/apache/accumulo/tserver/scan/NextBatchTask.java
##########
@@ -89,14 +88,16 @@ public void run() {
}
} catch (TooManyFilesException | SampleNotPresentException e) {
addResult(e);
- } catch (OutOfMemoryError ome) {
- Halt.halt("Ran out of memory scanning " + scanSession.extent + " for " +
scanSession.client,
- 1);
- addResult(ome);
- } catch (Throwable e) {
+ } catch (Exception e) {
Review comment:
Should this be more specific, like RuntimeException?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
