> 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. > > Hari Shreedharan wrote: > 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). > > Brock Noland wrote: > Hari, this is fine, but are you saying we shouldn't propagate or how does > this relate to the change? > > Hari Shreedharan wrote: > 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. > > Brock Noland wrote: > 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: > > ----------------------------------------------------------- > 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 > >
