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 [email protected] or file a JIRA ticket
with INFRA.
---
