Flume
  1. Flume
  2. FLUME-1134

LifecycleSupervisor.MonitorRunnable does not log runtime errors

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Duplicate
    • Affects Version/s: v1.1.0
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None

      Description

      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.

      1. FLUME-1134-0.patch
        5 kB
        Brock Noland
      2. FLUME-1134-1.patch
        5 kB
        Brock Noland

        Issue Links

          Activity

          Hide
          jiraposter@reviews.apache.org added a comment -

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          https://reviews.apache.org/r/4839/
          -----------------------------------------------------------

          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

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4839/ ----------------------------------------------------------- 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
          Hide
          Brock Noland added a comment -

          Patch from RB

          Show
          Brock Noland added a comment - Patch from RB
          Hide
          jiraposter@reviews.apache.org added a comment -

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          https://reviews.apache.org/r/4839/#review7123
          -----------------------------------------------------------

          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

          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

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4839/#review7123 ----------------------------------------------------------- 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 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
          Hide
          jiraposter@reviews.apache.org added a comment -

          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.

          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.

          • 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

          Show
          jiraposter@reviews.apache.org added a comment - 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. 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. 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
          Hide
          jiraposter@reviews.apache.org added a comment -

          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.

          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.

          • 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

          Show
          jiraposter@reviews.apache.org added a comment - 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. 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. 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
          Hide
          jiraposter@reviews.apache.org added a comment -

          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

          Show
          jiraposter@reviews.apache.org added a comment - 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
          Hide
          jiraposter@reviews.apache.org added a comment -

          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).

          Hari, this is fine, but are you saying we shouldn't propagate or how does this relate to the change?

          • 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

          Show
          jiraposter@reviews.apache.org added a comment - 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). Hari, this is fine, but are you saying we shouldn't propagate or how does this relate to the change? 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
          Hide
          jiraposter@reviews.apache.org added a comment -

          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?

          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.

          • 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

          Show
          jiraposter@reviews.apache.org added a comment - 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? 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. 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
          Hide
          jiraposter@reviews.apache.org added a comment -

          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

          Show
          jiraposter@reviews.apache.org added a comment - 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
          Hide
          jiraposter@reviews.apache.org added a comment -

          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

          Show
          jiraposter@reviews.apache.org added a comment - 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
          Hide
          Brock Noland added a comment -

          Attached patch from RB

          Show
          Brock Noland added a comment - Attached patch from RB
          Hide
          jiraposter@reviews.apache.org added a comment -

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          https://reviews.apache.org/r/4839/
          -----------------------------------------------------------

          (Updated 2012-04-29 14:54:30.474278)

          Review request for Flume.

          Changes
          -------

          Updated diff removing propagation.

          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 (updated)


          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

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4839/ ----------------------------------------------------------- (Updated 2012-04-29 14:54:30.474278) Review request for Flume. Changes ------- Updated diff removing propagation. 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 (updated) 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
          Hide
          jiraposter@reviews.apache.org added a comment -

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          https://reviews.apache.org/r/4839/#review7369
          -----------------------------------------------------------

          Ship it!

          +1
          Looks good!

          • Hari

          On 2012-04-29 14:54:30, Brock Noland wrote:

          -----------------------------------------------------------

          This is an automatically generated e-mail. To reply, visit:

          https://reviews.apache.org/r/4839/

          -----------------------------------------------------------

          (Updated 2012-04-29 14:54:30)

          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

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4839/#review7369 ----------------------------------------------------------- Ship it! +1 Looks good! Hari On 2012-04-29 14:54:30, Brock Noland wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4839/ ----------------------------------------------------------- (Updated 2012-04-29 14:54:30) 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

            People

            • Assignee:
              Brock Noland
              Reporter:
              Brock Noland
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development