brooklyn-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From aledsage <...@git.apache.org>
Subject [GitHub] brooklyn-server pull request #391: Enable BasicExecutionManager logging for ...
Date Thu, 20 Oct 2016 15:02:24 GMT
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/brooklyn-server/pull/391#discussion_r84303050
  
    --- Diff: core/src/main/java/org/apache/brooklyn/util/core/task/BasicExecutionManager.java
---
    @@ -536,7 +536,11 @@ public T call() {
                             // debug only here, because most submitters will handle failures
                             if (error instanceof InterruptedException || error instanceof
RuntimeInterruptedException) {
                                 log.debug("Detected interruption on task "+task+" (rethrowing)"
+
    -                                (Strings.isNonBlank(error.getMessage()) ? ": "+error.getMessage()
: ""));
    +                                    (Strings.isNonBlank(error.getMessage()) ? ": "+error.getMessage()
: ""));
    +                        } else if (error instanceof NullPointerException ||
    --- End diff --
    
    I agree with this, even though the code looks surprising! Here's the reasoning...
    
    We really do not want to swallow the stacktrace of exceptions. Doing so can make it extremely
hard to investigate/fix, particularly if that problem is reported by a user who hit it non-deterministically
(rather than by a Brooklyn developer during normal dev work). On the other hand, we don't
want to log exceptions repeatedly (particularly if that is from something like an SshFeed
that fails to reach a VM every 1 second).
    
    At this level in the code, we expect the common-case to be for the exception to be logged
by the person who invoked the task (they decide what level to log it at, etc). But for certain
errors that should never ever happen in a production use-case, it's better to risk logging
it repeatedly than never logging it at all (for example, a `NullPointerException`, etc). Such
errors suggest a configuration or programming error.
    
    The options considered were:
    * always log these special cases, accepting the duplication (done here)
    * always log all exceptions at debug (risking filling the log with repeated problems,
such as polling a dead VM every second)
    * leave as-was (but then the example of the underlying configuration issue causing the
NullPointerException would be really hard (or impossible for many) to fix!)
    * do something more complicated to guarantee the exception gets retrieved or logged. For
example, when the task is finalised, if no-one has called get() on the task in such a way
as would return the exception, then log it. But then the log message might have a completely
different time, so that sounds like a bad idea!


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

Mime
View raw message