> On 2012-04-23 06:35:54, Hari Shreedharan wrote: > > Looks good, if the intent was simply to log it. > > > > 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). > > > > 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. > > Hari Shreedharan wrote: > 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. > > Brock Noland wrote: > 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.
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). - 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: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/4839/ > ----------------------------------------------------------- > > (Updated 2012-04-22 23:23:14) > > > Review request for Flume. > > > Summary > ------- > > 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. > > > This addresses bug FLUME-1134. > https://issues.apache.org/jira/browse/FLUME-1134 > > > Diffs > ----- > > > flume-ng-core/src/main/java/org/apache/flume/lifecycle/LifecycleSupervisor.java > 2935e64 > > Diff: https://reviews.apache.org/r/4839/diff > > > Testing > ------- > > Logging only change. > > > Thanks, > > Brock > >
