> 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.
Thinking about this more, I do not see where we would propagate the error to? I think we should just remove the propagate. - Brock ----------------------------------------------------------- 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 > >
