[
https://issues.apache.org/jira/browse/FLUME-1134?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13259777#comment-13259777
]
[email protected] commented on FLUME-1134:
------------------------------------------------------
bq. On 2012-04-23 06:35:54, Hari Shreedharan wrote:
bq. > Looks good, if the intent was simply to log it.
bq. >
bq. > I am not sure of the intent here, is it just to log or to try and take
an action to fix the situation(the propogate call seems to suggest that you
want to get the exception thrown to one of the other threads or the process
terminated).
bq. >
bq. > If the intent is to really check for exceptions in each of the
monitorRunnables and take some action, like shutdown the monitorService or
something, we could have a scheduled thread execute every few seconds and call
monitorFutures.get(lifecyleAware).get() on all the futures stored in
monitorFutures map. If one of them died due to an exception we can catch it.
bq.
bq. Hari Shreedharan wrote:
bq. If one of them died due to an exception we can catch it. <-- By this
I meant, take any action as seen fit at the time.
bq.
bq. Brock Noland wrote:
bq. The intent is a small incremental improvement. Currently all
Throwables are eaten and not logged, so the intent is to log them. I am
propagating because it costs nothing to do and should the way the runnable is
created changes, say the implementation of ScheduledExecutorService changes,
they should be propagated.
bq.
bq. Hari Shreedharan wrote:
bq. Thanks for the explanation, Brock. It is highly unlikely that
ScheduledExecutorService will ever actually throw the exception which is thrown
by a runnable. This is because of the fact that it is impossible to determine
which thread to throw the exception to, since scheduled execution is
asynchronous (and the original thread which scheduled this is now doing
something else), which is the reason the Future.get() function throws the
exception which is thrown by the runnable(wrapped in an ExecutionException).
bq.
bq. Brock Noland wrote:
bq. Hari, this is fine, but are you saying we shouldn't propagate or how
does this relate to the change?
bq.
bq. Hari Shreedharan wrote:
bq. What I mean to say that calling propogate() here is pretty much a
no-op. If we really want to propogate then we should be polling the get
function periodically. I am ok with the change(which I mentioned in the
original comment), it should be pushed as soon as possible, just a bit
concerned whether propogate() is the right thing to do(since future behavior of
the ExecutorService is undefined), the right way to do it would be to poll the
get() function.
bq.
bq. Brock Noland wrote:
bq. Thinking about this more, I do not see where we would propagate the
error to? I think we should just remove the propagate.
Agreed, it is not advisable to propagate exceptions from an asynchronous
operation, since we do not control how the executor service will handle it
later. Also, propagating it upwards will cause that runnable to never be
executed again(this is what happens currently though. According to the
javadocs: If any execution of the task encounters an exception, subsequent
executions are suppressed)
- Hari
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4839/#review7123
-----------------------------------------------------------
On 2012-04-22 23:23:14, Brock Noland wrote:
bq.
bq. -----------------------------------------------------------
bq. This is an automatically generated e-mail. To reply, visit:
bq. https://reviews.apache.org/r/4839/
bq. -----------------------------------------------------------
bq.
bq. (Updated 2012-04-22 23:23:14)
bq.
bq.
bq. Review request for Flume.
bq.
bq.
bq. Summary
bq. -------
bq.
bq. Currently we use an ScheduledExecutorService which eats any throwable from
MonitoredRunnable. The change is to log the error and then propagate in case
the caller implementation should change.
bq.
bq.
bq. This addresses bug FLUME-1134.
bq. https://issues.apache.org/jira/browse/FLUME-1134
bq.
bq.
bq. Diffs
bq. -----
bq.
bq.
flume-ng-core/src/main/java/org/apache/flume/lifecycle/LifecycleSupervisor.java
2935e64
bq.
bq. Diff: https://reviews.apache.org/r/4839/diff
bq.
bq.
bq. Testing
bq. -------
bq.
bq. Logging only change.
bq.
bq.
bq. Thanks,
bq.
bq. Brock
bq.
bq.
> LifecycleSupervisor.MonitorRunnable does not log runtime errors
> ---------------------------------------------------------------
>
> Key: FLUME-1134
> URL: https://issues.apache.org/jira/browse/FLUME-1134
> Project: Flume
> Issue Type: Bug
> Affects Versions: v1.1.0
> Reporter: Brock Noland
> Assignee: Brock Noland
> Attachments: FLUME-1134-0.patch
>
>
> LifecycleSupervisor.MonitorRunnable does not log RuntimeException/Error but
> the class we use to execute these runnables eats the exceptions. The run
> method should be wrapped in a try catch and log.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators:
https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
