ctubbsii commented on a change in pull request #1818:
URL: https://github.com/apache/accumulo/pull/1818#discussion_r557014219
##########
File path:
core/src/main/java/org/apache/accumulo/core/util/threads/AccumuloUncaughtExceptionHandler.java
##########
@@ -16,20 +16,36 @@
* specific language governing permissions and limitations
* under the License.
*/
-package org.apache.accumulo.core.util;
+package org.apache.accumulo.core.util.threads;
import java.lang.Thread.UncaughtExceptionHandler;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
-public class AccumuloUncaughtExceptionHandler implements
UncaughtExceptionHandler {
+/**
+ * UncaughtExceptionHandler that logs all Exceptions and Errors thrown from a
Thread. If an Error is
+ * thrown, halt the JVM.
+ *
+ */
+class AccumuloUncaughtExceptionHandler implements UncaughtExceptionHandler {
- private static final Logger log =
LoggerFactory.getLogger(AccumuloUncaughtExceptionHandler.class);
+ private static final Logger LOG =
LoggerFactory.getLogger(AccumuloUncaughtExceptionHandler.class);
@Override
public void uncaughtException(Thread t, Throwable e) {
- log.error(String.format("Caught an exception in %s. Shutting down.", t),
e);
+ if (e instanceof Exception) {
+ LOG.error("Caught an Exception in {}. Thread is dead.", t, e);
+ } else if (e instanceof Error) {
+ try {
+ e.printStackTrace();
+ System.err.println("Error thrown in thread: " + t + ", halting VM.");
+ } catch (Throwable e1) {
+ // If e == OutOfMemoryError, then it's probably that another Error
might be
+ // thrown when trying to print to System.err.
+ } finally {
+ Runtime.getRuntime().halt(-1);
Review comment:
> a miss-configured context that leads to a UnsupportedClassVersionError
or an IOError on a particular file
I would argue that misconfigured class paths are much more serious of an
issue than would be implied by trying to proceed in spite of errors caused by
them. We should *not* be trying to proceed when an error occurs as the result
of a misconfigured class path. If you can't trust the class path, you can't
trust anything, and it is not safe to proceed.
As for IOError, see [Mark Reinhold's explanation for
that](http://cafe.elharo.com/blogroll/undocumented-changes-in-java-6-mustang-ioerror/#comment-160)
; he specifically explains that it was added to be used in situations where an
*unrecoverable error* (his words) occurs. Suggesting that we should try to
recover from situations that are definition-ally unrecoverable seems a bit
unreasonable to me.
> One bad iterator/edge case should not be able to take a cluster down.
I understand the motivation for pursuing this. However, thinking about worst
case scenarios, I can imagine far worse things than being temporarily offline
(data corruption, hijacking by malware, etc.) due to administrator error
deploying a bad iterator.
I think that baking in special handling for these (what should be
exceedingly rare) edge cases would be a mistake for a number of reasons.
However, persuasive arguments in favor of, or against, such a thing can be made
in a subsequent effort, if there is a critical demand for such a thing. There's
no need to block this PR on that. It would actually be much simpler to discuss
that sort of thing on its own, rather than bundled with the other changes in
this PR.
----------------------------------------------------------------
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]
