Flume
  1. Flume
  2. FLUME-984

SinkRunner should catch unhanded exceptions and log them like PollingSourceRunner

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: v1.0.0
    • Fix Version/s: v1.1.0
    • Component/s: Sinks+Sources
    • Labels:
      None

      Description

      If a Sink throws any exception but EventDeliveryException the sink will die. We should catch the exception, perhaps backoff a bit, and retry like PollingSourceRunner.

      1. FLUME-984-4.patch
        4 kB
        Brock Noland
      2. FLUME-984-2.patch
        4 kB
        Brock Noland
      3. FLUME-984-1.patch
        3 kB
        Brock Noland
      4. FLUME-984-0.patch
        1 kB
        Brock Noland

        Issue Links

          Activity

          Hide
          Brock Noland added a comment -

          Longterm I wonder if we should a service fail N times we close and it restart. I don't feel we have the items in place today to do this. The SinkRunner could instead of being passed a sink, be passed a Supplier<Sink> for example.

          Show
          Brock Noland added a comment - Longterm I wonder if we should a service fail N times we close and it restart. I don't feel we have the items in place today to do this. The SinkRunner could instead of being passed a sink, be passed a Supplier<Sink> for example.
          Hide
          Arvind Prabhakar added a comment -

          @Juhani - Sorry if I came across as not being sensitive to the discussion here. Please re-open the Jira to continue the discussion or better still, open another Jira to address exception handling throughout the project.

          I do feel that putting logic into exceptions is a choice that causes more unpredictable behavior and is hard to implement. Hence my suggestion to Brock to remove it. Since he updated the patch, I felt it was ready for commit.

          Show
          Arvind Prabhakar added a comment - @Juhani - Sorry if I came across as not being sensitive to the discussion here. Please re-open the Jira to continue the discussion or better still, open another Jira to address exception handling throughout the project. I do feel that putting logic into exceptions is a choice that causes more unpredictable behavior and is hard to implement. Hence my suggestion to Brock to remove it. Since he updated the patch, I felt it was ready for commit.
          Hide
          Juhani Connolly added a comment -

          I'm somewhat disappointed this was rushed through despite clear disagreement without discussing the exception handling methods. While I think there is some merit to Arvinds argument, I think it is the task of any outside developer to read the javadoc and comply with it when developing a component.

          As is, we cannot tell the difference between an error that would mean removing from a balancing or failover pool, and one that is just a one off. Beating away on a dead sink(especially one that makes several retries, blocking a thread) is a waste of resources.

          By specifying separate exceptions for temporary delivery problems and for more permanent ones, we could provide a clear interface to users, while our current implementation now leaves us wide open for bad implementations where people just throw whatever exception and it gets more or less ignored(just a log entry).

          Show
          Juhani Connolly added a comment - I'm somewhat disappointed this was rushed through despite clear disagreement without discussing the exception handling methods. While I think there is some merit to Arvinds argument, I think it is the task of any outside developer to read the javadoc and comply with it when developing a component. As is, we cannot tell the difference between an error that would mean removing from a balancing or failover pool, and one that is just a one off. Beating away on a dead sink(especially one that makes several retries, blocking a thread) is a waste of resources. By specifying separate exceptions for temporary delivery problems and for more permanent ones, we could provide a clear interface to users, while our current implementation now leaves us wide open for bad implementations where people just throw whatever exception and it gets more or less ignored(just a log entry).
          Hide
          jiraposter@reviews.apache.org added a comment -

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

          Ship it!

          +1

          • Arvind

          On 2012-03-12 19:10:23, Brock Noland wrote:

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

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

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

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

          (Updated 2012-03-12 19:10:23)

          Review request for Flume.

          Summary

          -------

          Catches, logs, and sleeps when a general exception occurs. This is similar to PollingSourceRunner.

          This addresses bug FLUME-984.

          https://issues.apache.org/jira/browse/FLUME-984

          Diffs

          -----

          flume-ng-core/src/main/java/org/apache/flume/SinkRunner.java ee75b54

          flume-ng-core/src/main/java/org/apache/flume/source/PollableSourceRunner.java 264fce1

          Diff: https://reviews.apache.org/r/3980/diff

          Testing

          -------

          All unit tests pass.

          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/3980/#review5849 ----------------------------------------------------------- Ship it! +1 Arvind On 2012-03-12 19:10:23, Brock Noland wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3980/ ----------------------------------------------------------- (Updated 2012-03-12 19:10:23) Review request for Flume. Summary ------- Catches, logs, and sleeps when a general exception occurs. This is similar to PollingSourceRunner. This addresses bug FLUME-984 . https://issues.apache.org/jira/browse/FLUME-984 Diffs ----- flume-ng-core/src/main/java/org/apache/flume/SinkRunner.java ee75b54 flume-ng-core/src/main/java/org/apache/flume/source/PollableSourceRunner.java 264fce1 Diff: https://reviews.apache.org/r/3980/diff Testing ------- All unit tests pass. Thanks, Brock
          Hide
          Arvind Prabhakar added a comment -

          Patch committed. Thanks Brock!

          Show
          Arvind Prabhakar added a comment - Patch committed. 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/3980/
          -----------------------------------------------------------

          (Updated 2012-03-12 19:10:23.481570)

          Review request for Flume.

          Changes
          -------

          Updated patch based on comments.

          Summary
          -------

          Catches, logs, and sleeps when a general exception occurs. This is similar to PollingSourceRunner.

          This addresses bug FLUME-984.
          https://issues.apache.org/jira/browse/FLUME-984

          Diffs (updated)


          flume-ng-core/src/main/java/org/apache/flume/SinkRunner.java ee75b54
          flume-ng-core/src/main/java/org/apache/flume/source/PollableSourceRunner.java 264fce1

          Diff: https://reviews.apache.org/r/3980/diff

          Testing
          -------

          All unit tests pass.

          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/3980/ ----------------------------------------------------------- (Updated 2012-03-12 19:10:23.481570) Review request for Flume. Changes ------- Updated patch based on comments. Summary ------- Catches, logs, and sleeps when a general exception occurs. This is similar to PollingSourceRunner. This addresses bug FLUME-984 . https://issues.apache.org/jira/browse/FLUME-984 Diffs (updated) flume-ng-core/src/main/java/org/apache/flume/SinkRunner.java ee75b54 flume-ng-core/src/main/java/org/apache/flume/source/PollableSourceRunner.java 264fce1 Diff: https://reviews.apache.org/r/3980/diff Testing ------- All unit tests pass. Thanks, Brock
          Hide
          Brock Noland added a comment -

          Agreed upon patch from RB.

          Show
          Brock Noland added a comment - Agreed upon 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/3980/#review5845
          -----------------------------------------------------------

          Ship it!

          +1. Some minor comments below. Please address those if possible and attach the updated patch to the Jira.

          flume-ng-core/src/main/java/org/apache/flume/SinkRunner.java
          <https://reviews.apache.org/r/3980/#comment12738>

          Longer than 80 char.

          flume-ng-core/src/main/java/org/apache/flume/source/PollableSourceRunner.java
          <https://reviews.apache.org/r/3980/#comment12739>

          Longer than 80 char.

          • Arvind

          On 2012-03-12 18:52:15, Brock Noland wrote:

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

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

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

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

          (Updated 2012-03-12 18:52:15)

          Review request for Flume.

          Summary

          -------

          Catches, logs, and sleeps when a general exception occurs. This is similar to PollingSourceRunner.

          This addresses bug FLUME-984.

          https://issues.apache.org/jira/browse/FLUME-984

          Diffs

          -----

          flume-ng-core/src/main/java/org/apache/flume/SinkRunner.java ee75b54

          flume-ng-core/src/main/java/org/apache/flume/source/PollableSourceRunner.java 264fce1

          Diff: https://reviews.apache.org/r/3980/diff

          Testing

          -------

          All unit tests pass.

          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/3980/#review5845 ----------------------------------------------------------- Ship it! +1. Some minor comments below. Please address those if possible and attach the updated patch to the Jira. flume-ng-core/src/main/java/org/apache/flume/SinkRunner.java < https://reviews.apache.org/r/3980/#comment12738 > Longer than 80 char. flume-ng-core/src/main/java/org/apache/flume/source/PollableSourceRunner.java < https://reviews.apache.org/r/3980/#comment12739 > Longer than 80 char. Arvind On 2012-03-12 18:52:15, Brock Noland wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3980/ ----------------------------------------------------------- (Updated 2012-03-12 18:52:15) Review request for Flume. Summary ------- Catches, logs, and sleeps when a general exception occurs. This is similar to PollingSourceRunner. This addresses bug FLUME-984 . https://issues.apache.org/jira/browse/FLUME-984 Diffs ----- flume-ng-core/src/main/java/org/apache/flume/SinkRunner.java ee75b54 flume-ng-core/src/main/java/org/apache/flume/source/PollableSourceRunner.java 264fce1 Diff: https://reviews.apache.org/r/3980/diff Testing ------- All unit tests pass. Thanks, Brock
          Hide
          jiraposter@reviews.apache.org added a comment -

          On 2012-03-12 17:18:41, Arvind Prabhakar wrote:

          > Thanks for the patch Brock.

          >

          > I am not in favor of an interface to be implemented by the exception to signal any specific cause. The reason being that it will be hard and error prone for us to bring this into compliance specially for components developed outside of Flume. Any explicit contract that cannot be enforced is likely to become a cause of headaches in the field. Hence, my suggestion is to do the following:

          >

          > * Remove the FatalException interface

          > * Update the runners to catch(Exception) once they have dealt with all the known exception types, log it and do a BACKOFF wait simulation.

          >

          > This will ensure that the runners do not bail out if an exception occurs and keep retrying. This may be futile in some cases but would make sense in other cases as once a process/thread exits there is no restoring it without more complex logic up top or via manual intervention.

          FatalException is removed.

          I took the "Update the Runners" comment to mean that both SinkRunner and PollableSourceRunner should have the logic for dealing with exceptions. Hopefully I interpreted that correct. I think that both these runners having the same logic makes sense and it will also obsolete FLUME-982.

          • Brock

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

          On 2012-03-12 18:52:15, Brock Noland wrote:

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

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

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

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

          (Updated 2012-03-12 18:52:15)

          Review request for Flume.

          Summary

          -------

          Catches, logs, and sleeps when a general exception occurs. This is similar to PollingSourceRunner.

          This addresses bug FLUME-984.

          https://issues.apache.org/jira/browse/FLUME-984

          Diffs

          -----

          flume-ng-core/src/main/java/org/apache/flume/SinkRunner.java ee75b54

          flume-ng-core/src/main/java/org/apache/flume/source/PollableSourceRunner.java 264fce1

          Diff: https://reviews.apache.org/r/3980/diff

          Testing

          -------

          All unit tests pass.

          Thanks,

          Brock

          Show
          jiraposter@reviews.apache.org added a comment - On 2012-03-12 17:18:41, Arvind Prabhakar wrote: > Thanks for the patch Brock. > > I am not in favor of an interface to be implemented by the exception to signal any specific cause. The reason being that it will be hard and error prone for us to bring this into compliance specially for components developed outside of Flume. Any explicit contract that cannot be enforced is likely to become a cause of headaches in the field. Hence, my suggestion is to do the following: > > * Remove the FatalException interface > * Update the runners to catch(Exception) once they have dealt with all the known exception types, log it and do a BACKOFF wait simulation. > > This will ensure that the runners do not bail out if an exception occurs and keep retrying. This may be futile in some cases but would make sense in other cases as once a process/thread exits there is no restoring it without more complex logic up top or via manual intervention. FatalException is removed. I took the "Update the Runners" comment to mean that both SinkRunner and PollableSourceRunner should have the logic for dealing with exceptions. Hopefully I interpreted that correct. I think that both these runners having the same logic makes sense and it will also obsolete FLUME-982 . Brock ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3980/#review5837 ----------------------------------------------------------- On 2012-03-12 18:52:15, Brock Noland wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3980/ ----------------------------------------------------------- (Updated 2012-03-12 18:52:15) Review request for Flume. Summary ------- Catches, logs, and sleeps when a general exception occurs. This is similar to PollingSourceRunner. This addresses bug FLUME-984 . https://issues.apache.org/jira/browse/FLUME-984 Diffs ----- flume-ng-core/src/main/java/org/apache/flume/SinkRunner.java ee75b54 flume-ng-core/src/main/java/org/apache/flume/source/PollableSourceRunner.java 264fce1 Diff: https://reviews.apache.org/r/3980/diff Testing ------- All unit tests pass. 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/3980/
          -----------------------------------------------------------

          (Updated 2012-03-12 18:52:15.700886)

          Review request for Flume.

          Changes
          -------

          Update based on review.

          Summary
          -------

          Catches, logs, and sleeps when a general exception occurs. This is similar to PollingSourceRunner.

          This addresses bug FLUME-984.
          https://issues.apache.org/jira/browse/FLUME-984

          Diffs (updated)


          flume-ng-core/src/main/java/org/apache/flume/SinkRunner.java ee75b54
          flume-ng-core/src/main/java/org/apache/flume/source/PollableSourceRunner.java 264fce1

          Diff: https://reviews.apache.org/r/3980/diff

          Testing
          -------

          All unit tests pass.

          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/3980/ ----------------------------------------------------------- (Updated 2012-03-12 18:52:15.700886) Review request for Flume. Changes ------- Update based on review. Summary ------- Catches, logs, and sleeps when a general exception occurs. This is similar to PollingSourceRunner. This addresses bug FLUME-984 . https://issues.apache.org/jira/browse/FLUME-984 Diffs (updated) flume-ng-core/src/main/java/org/apache/flume/SinkRunner.java ee75b54 flume-ng-core/src/main/java/org/apache/flume/source/PollableSourceRunner.java 264fce1 Diff: https://reviews.apache.org/r/3980/diff Testing ------- All unit tests pass. 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/3980/#review5837
          -----------------------------------------------------------

          Thanks for the patch Brock.

          I am not in favor of an interface to be implemented by the exception to signal any specific cause. The reason being that it will be hard and error prone for us to bring this into compliance specially for components developed outside of Flume. Any explicit contract that cannot be enforced is likely to become a cause of headaches in the field. Hence, my suggestion is to do the following:

          • Remove the FatalException interface
          • Update the runners to catch(Exception) once they have dealt with all the known exception types, log it and do a BACKOFF wait simulation.

          This will ensure that the runners do not bail out if an exception occurs and keep retrying. This may be futile in some cases but would make sense in other cases as once a process/thread exits there is no restoring it without more complex logic up top or via manual intervention.

          • Arvind

          On 2012-02-29 10:42:17, Brock Noland wrote:

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

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

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

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

          (Updated 2012-02-29 10:42:17)

          Review request for Flume.

          Summary

          -------

          Catches, logs, and sleeps when a general exception occurs. This is similar to PollingSourceRunner.

          This addresses bug FLUME-984.

          https://issues.apache.org/jira/browse/FLUME-984

          Diffs

          -----

          flume-ng-core/src/main/java/org/apache/flume/FatalException.java PRE-CREATION

          flume-ng-core/src/main/java/org/apache/flume/source/PollableSourceRunner.java 264fce1

          flume-ng-core/src/main/java/org/apache/flume/SinkRunner.java 9e46d87

          Diff: https://reviews.apache.org/r/3980/diff

          Testing

          -------

          All unit tests pass.

          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/3980/#review5837 ----------------------------------------------------------- Thanks for the patch Brock. I am not in favor of an interface to be implemented by the exception to signal any specific cause. The reason being that it will be hard and error prone for us to bring this into compliance specially for components developed outside of Flume. Any explicit contract that cannot be enforced is likely to become a cause of headaches in the field. Hence, my suggestion is to do the following: Remove the FatalException interface Update the runners to catch(Exception) once they have dealt with all the known exception types, log it and do a BACKOFF wait simulation. This will ensure that the runners do not bail out if an exception occurs and keep retrying. This may be futile in some cases but would make sense in other cases as once a process/thread exits there is no restoring it without more complex logic up top or via manual intervention. Arvind On 2012-02-29 10:42:17, Brock Noland wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3980/ ----------------------------------------------------------- (Updated 2012-02-29 10:42:17) Review request for Flume. Summary ------- Catches, logs, and sleeps when a general exception occurs. This is similar to PollingSourceRunner. This addresses bug FLUME-984 . https://issues.apache.org/jira/browse/FLUME-984 Diffs ----- flume-ng-core/src/main/java/org/apache/flume/FatalException.java PRE-CREATION flume-ng-core/src/main/java/org/apache/flume/source/PollableSourceRunner.java 264fce1 flume-ng-core/src/main/java/org/apache/flume/SinkRunner.java 9e46d87 Diff: https://reviews.apache.org/r/3980/diff Testing ------- All unit tests pass. Thanks, Brock
          Hide
          jiraposter@reviews.apache.org added a comment -

          On 2012-03-05 03:20:15, Juhani Connolly wrote:

          > This seems fine... However without the change being propagated to the sinks to make them throw appropriate errors it will be misleading. Do you think you could update the sinks/processors to account for the new exception? If not, we can just push this and I'll open a separate ticket to do that...

          You mean update the sink signature so that they know they can throw FatalException?

          • Brock

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

          On 2012-02-29 10:42:17, Brock Noland wrote:

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

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

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

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

          (Updated 2012-02-29 10:42:17)

          Review request for Flume.

          Summary

          -------

          Catches, logs, and sleeps when a general exception occurs. This is similar to PollingSourceRunner.

          This addresses bug FLUME-984.

          https://issues.apache.org/jira/browse/FLUME-984

          Diffs

          -----

          flume-ng-core/src/main/java/org/apache/flume/FatalException.java PRE-CREATION

          flume-ng-core/src/main/java/org/apache/flume/source/PollableSourceRunner.java 264fce1

          flume-ng-core/src/main/java/org/apache/flume/SinkRunner.java 9e46d87

          Diff: https://reviews.apache.org/r/3980/diff

          Testing

          -------

          All unit tests pass.

          Thanks,

          Brock

          Show
          jiraposter@reviews.apache.org added a comment - On 2012-03-05 03:20:15, Juhani Connolly wrote: > This seems fine... However without the change being propagated to the sinks to make them throw appropriate errors it will be misleading. Do you think you could update the sinks/processors to account for the new exception? If not, we can just push this and I'll open a separate ticket to do that... You mean update the sink signature so that they know they can throw FatalException? Brock ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3980/#review5608 ----------------------------------------------------------- On 2012-02-29 10:42:17, Brock Noland wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3980/ ----------------------------------------------------------- (Updated 2012-02-29 10:42:17) Review request for Flume. Summary ------- Catches, logs, and sleeps when a general exception occurs. This is similar to PollingSourceRunner. This addresses bug FLUME-984 . https://issues.apache.org/jira/browse/FLUME-984 Diffs ----- flume-ng-core/src/main/java/org/apache/flume/FatalException.java PRE-CREATION flume-ng-core/src/main/java/org/apache/flume/source/PollableSourceRunner.java 264fce1 flume-ng-core/src/main/java/org/apache/flume/SinkRunner.java 9e46d87 Diff: https://reviews.apache.org/r/3980/diff Testing ------- All unit tests pass. Thanks, Brock
          Hide
          jiraposter@reviews.apache.org added a comment -

          On 2012-03-11 07:09:15, Mike Percy wrote:

          > Hi Brock, great catch on the lack of a catch-all Exception handler in SinkRunner. Regarding FatalException, I think it should inherit from FlumeException since the other exceptions do except for EventDeliveryException, which should probably be changed to do that. (see also the thread on flume-dev about checked vs unchecked exceptions... not sure if we exactly came to consensus on that and if not we should aim for that once and for all to maintain consistency going fwd)

          >

          > Regarding the use of FatalException, can you elaborate a little on the use case for it? When would the system want to allow a Source or Sink to force the runner to quit?

          I think the use case is that you want to throw a terminal exception such that the sink will not be retried.

          If implemented, I think that inheriting from FlumeException would make sense. Let me know how you feel about the use case and then I will update the patch appropriately.

          • Brock

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

          On 2012-02-29 10:42:17, Brock Noland wrote:

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

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

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

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

          (Updated 2012-02-29 10:42:17)

          Review request for Flume.

          Summary

          -------

          Catches, logs, and sleeps when a general exception occurs. This is similar to PollingSourceRunner.

          This addresses bug FLUME-984.

          https://issues.apache.org/jira/browse/FLUME-984

          Diffs

          -----

          flume-ng-core/src/main/java/org/apache/flume/FatalException.java PRE-CREATION

          flume-ng-core/src/main/java/org/apache/flume/source/PollableSourceRunner.java 264fce1

          flume-ng-core/src/main/java/org/apache/flume/SinkRunner.java 9e46d87

          Diff: https://reviews.apache.org/r/3980/diff

          Testing

          -------

          All unit tests pass.

          Thanks,

          Brock

          Show
          jiraposter@reviews.apache.org added a comment - On 2012-03-11 07:09:15, Mike Percy wrote: > Hi Brock, great catch on the lack of a catch-all Exception handler in SinkRunner. Regarding FatalException, I think it should inherit from FlumeException since the other exceptions do except for EventDeliveryException, which should probably be changed to do that. (see also the thread on flume-dev about checked vs unchecked exceptions... not sure if we exactly came to consensus on that and if not we should aim for that once and for all to maintain consistency going fwd) > > Regarding the use of FatalException, can you elaborate a little on the use case for it? When would the system want to allow a Source or Sink to force the runner to quit? I think the use case is that you want to throw a terminal exception such that the sink will not be retried. If implemented, I think that inheriting from FlumeException would make sense. Let me know how you feel about the use case and then I will update the patch appropriately. Brock ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3980/#review5824 ----------------------------------------------------------- On 2012-02-29 10:42:17, Brock Noland wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3980/ ----------------------------------------------------------- (Updated 2012-02-29 10:42:17) Review request for Flume. Summary ------- Catches, logs, and sleeps when a general exception occurs. This is similar to PollingSourceRunner. This addresses bug FLUME-984 . https://issues.apache.org/jira/browse/FLUME-984 Diffs ----- flume-ng-core/src/main/java/org/apache/flume/FatalException.java PRE-CREATION flume-ng-core/src/main/java/org/apache/flume/source/PollableSourceRunner.java 264fce1 flume-ng-core/src/main/java/org/apache/flume/SinkRunner.java 9e46d87 Diff: https://reviews.apache.org/r/3980/diff Testing ------- All unit tests pass. 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/3980/#review5824
          -----------------------------------------------------------

          Hi Brock, great catch on the lack of a catch-all Exception handler in SinkRunner. Regarding FatalException, I think it should inherit from FlumeException since the other exceptions do except for EventDeliveryException, which should probably be changed to do that. (see also the thread on flume-dev about checked vs unchecked exceptions... not sure if we exactly came to consensus on that and if not we should aim for that once and for all to maintain consistency going fwd)

          Regarding the use of FatalException, can you elaborate a little on the use case for it? When would the system want to allow a Source or Sink to force the runner to quit?

          • Mike

          On 2012-02-29 10:42:17, Brock Noland wrote:

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

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

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

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

          (Updated 2012-02-29 10:42:17)

          Review request for Flume.

          Summary

          -------

          Catches, logs, and sleeps when a general exception occurs. This is similar to PollingSourceRunner.

          This addresses bug FLUME-984.

          https://issues.apache.org/jira/browse/FLUME-984

          Diffs

          -----

          flume-ng-core/src/main/java/org/apache/flume/FatalException.java PRE-CREATION

          flume-ng-core/src/main/java/org/apache/flume/source/PollableSourceRunner.java 264fce1

          flume-ng-core/src/main/java/org/apache/flume/SinkRunner.java 9e46d87

          Diff: https://reviews.apache.org/r/3980/diff

          Testing

          -------

          All unit tests pass.

          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/3980/#review5824 ----------------------------------------------------------- Hi Brock, great catch on the lack of a catch-all Exception handler in SinkRunner. Regarding FatalException, I think it should inherit from FlumeException since the other exceptions do except for EventDeliveryException, which should probably be changed to do that. (see also the thread on flume-dev about checked vs unchecked exceptions... not sure if we exactly came to consensus on that and if not we should aim for that once and for all to maintain consistency going fwd) Regarding the use of FatalException, can you elaborate a little on the use case for it? When would the system want to allow a Source or Sink to force the runner to quit? Mike On 2012-02-29 10:42:17, Brock Noland wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3980/ ----------------------------------------------------------- (Updated 2012-02-29 10:42:17) Review request for Flume. Summary ------- Catches, logs, and sleeps when a general exception occurs. This is similar to PollingSourceRunner. This addresses bug FLUME-984 . https://issues.apache.org/jira/browse/FLUME-984 Diffs ----- flume-ng-core/src/main/java/org/apache/flume/FatalException.java PRE-CREATION flume-ng-core/src/main/java/org/apache/flume/source/PollableSourceRunner.java 264fce1 flume-ng-core/src/main/java/org/apache/flume/SinkRunner.java 9e46d87 Diff: https://reviews.apache.org/r/3980/diff Testing ------- All unit tests pass. Thanks, Brock
          Hide
          Brock Noland added a comment -

          Good call, I will update.

          Show
          Brock Noland added a comment - Good call, I will update.
          Hide
          jiraposter@reviews.apache.org added a comment -

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

          This seems fine... However without the change being propagated to the sinks to make them throw appropriate errors it will be misleading. Do you think you could update the sinks/processors to account for the new exception? If not, we can just push this and I'll open a separate ticket to do that...

          • Juhani

          On 2012-02-29 10:42:17, Brock Noland wrote:

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

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

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

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

          (Updated 2012-02-29 10:42:17)

          Review request for Flume.

          Summary

          -------

          Catches, logs, and sleeps when a general exception occurs. This is similar to PollingSourceRunner.

          This addresses bug FLUME-984.

          https://issues.apache.org/jira/browse/FLUME-984

          Diffs

          -----

          flume-ng-core/src/main/java/org/apache/flume/FatalException.java PRE-CREATION

          flume-ng-core/src/main/java/org/apache/flume/source/PollableSourceRunner.java 264fce1

          flume-ng-core/src/main/java/org/apache/flume/SinkRunner.java 9e46d87

          Diff: https://reviews.apache.org/r/3980/diff

          Testing

          -------

          All unit tests pass.

          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/3980/#review5608 ----------------------------------------------------------- This seems fine... However without the change being propagated to the sinks to make them throw appropriate errors it will be misleading. Do you think you could update the sinks/processors to account for the new exception? If not, we can just push this and I'll open a separate ticket to do that... Juhani On 2012-02-29 10:42:17, Brock Noland wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3980/ ----------------------------------------------------------- (Updated 2012-02-29 10:42:17) Review request for Flume. Summary ------- Catches, logs, and sleeps when a general exception occurs. This is similar to PollingSourceRunner. This addresses bug FLUME-984 . https://issues.apache.org/jira/browse/FLUME-984 Diffs ----- flume-ng-core/src/main/java/org/apache/flume/FatalException.java PRE-CREATION flume-ng-core/src/main/java/org/apache/flume/source/PollableSourceRunner.java 264fce1 flume-ng-core/src/main/java/org/apache/flume/SinkRunner.java 9e46d87 Diff: https://reviews.apache.org/r/3980/diff Testing ------- All unit tests pass. 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/3980/
          -----------------------------------------------------------

          (Updated 2012-02-29 10:42:17.590582)

          Review request for Flume.

          Changes
          -------

          Renamed "fatal" interface to FatalException.
          Implemented same fatal functionality to PollingSourceRunner.
          Exceptions were counted under different names. Now consolidated.

          Summary
          -------

          Catches, logs, and sleeps when a general exception occurs. This is similar to PollingSourceRunner.

          This addresses bug FLUME-984.
          https://issues.apache.org/jira/browse/FLUME-984

          Diffs (updated)


          flume-ng-core/src/main/java/org/apache/flume/FatalException.java PRE-CREATION
          flume-ng-core/src/main/java/org/apache/flume/source/PollableSourceRunner.java 264fce1
          flume-ng-core/src/main/java/org/apache/flume/SinkRunner.java 9e46d87

          Diff: https://reviews.apache.org/r/3980/diff

          Testing
          -------

          All unit tests pass.

          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/3980/ ----------------------------------------------------------- (Updated 2012-02-29 10:42:17.590582) Review request for Flume. Changes ------- Renamed "fatal" interface to FatalException. Implemented same fatal functionality to PollingSourceRunner. Exceptions were counted under different names. Now consolidated. Summary ------- Catches, logs, and sleeps when a general exception occurs. This is similar to PollingSourceRunner. This addresses bug FLUME-984 . https://issues.apache.org/jira/browse/FLUME-984 Diffs (updated) flume-ng-core/src/main/java/org/apache/flume/FatalException.java PRE-CREATION flume-ng-core/src/main/java/org/apache/flume/source/PollableSourceRunner.java 264fce1 flume-ng-core/src/main/java/org/apache/flume/SinkRunner.java 9e46d87 Diff: https://reviews.apache.org/r/3980/diff Testing ------- All unit tests pass. Thanks, Brock
          Hide
          Brock Noland added a comment -

          Attached is the patch from the review.

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

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

          (Updated 2012-02-22 01:26:59.240182)

          Review request for Flume.

          Changes
          -------

          Updated diff based on feedback. Now if Sink throws a FlumePermanentFailureException the SinkRunner will terminate.

          Summary
          -------

          Catches, logs, and sleeps when a general exception occurs. This is similar to PollingSourceRunner.

          This addresses bug FLUME-984.
          https://issues.apache.org/jira/browse/FLUME-984

          Diffs (updated)


          flume-ng-core/src/main/java/org/apache/flume/FlumePermanentFailureException.java PRE-CREATION
          flume-ng-core/src/main/java/org/apache/flume/SinkRunner.java 9e46d87

          Diff: https://reviews.apache.org/r/3980/diff

          Testing
          -------

          All unit tests pass.

          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/3980/ ----------------------------------------------------------- (Updated 2012-02-22 01:26:59.240182) Review request for Flume. Changes ------- Updated diff based on feedback. Now if Sink throws a FlumePermanentFailureException the SinkRunner will terminate. Summary ------- Catches, logs, and sleeps when a general exception occurs. This is similar to PollingSourceRunner. This addresses bug FLUME-984 . https://issues.apache.org/jira/browse/FLUME-984 Diffs (updated) flume-ng-core/src/main/java/org/apache/flume/FlumePermanentFailureException.java PRE-CREATION flume-ng-core/src/main/java/org/apache/flume/SinkRunner.java 9e46d87 Diff: https://reviews.apache.org/r/3980/diff Testing ------- All unit tests pass. Thanks, Brock
          Hide
          Brock Noland added a comment -

          I updated the patch. Assuming that this or a similar change is committed we can update the sinks/source runners to respect the permanent failure condition.

          Show
          Brock Noland added a comment - I updated the patch. Assuming that this or a similar change is committed we can update the sinks/source runners to respect the permanent failure condition.
          Hide
          Brock Noland added a comment - - edited

          I think it's better than the current impl. However, yes I do prefer the scenario where a sink/source can throw and error which should not be retried.

          Show
          Brock Noland added a comment - - edited I think it's better than the current impl. However, yes I do prefer the scenario where a sink/source can throw and error which should not be retried.
          Hide
          Juhani Connolly added a comment -

          I'm not sure if just logging all exceptions and leaving the sink runner(and repeatedly failing for certain failure patterns) is the best choice.

          There should definitely be some kind of unified behavior to all sinks regarding when and how they fail, along with how they should be handled.

          Perhaps we should add an exception that sinks can throw to indicate they are unrecoverable?

          Show
          Juhani Connolly added a comment - I'm not sure if just logging all exceptions and leaving the sink runner(and repeatedly failing for certain failure patterns) is the best choice. There should definitely be some kind of unified behavior to all sinks regarding when and how they fail, along with how they should be handled. Perhaps we should add an exception that sinks can throw to indicate they are unrecoverable?
          Hide
          Brock Noland added a comment -

          Marking "Patch Available"

          Show
          Brock Noland added a comment - Marking "Patch Available"
          Hide
          Brock Noland added a comment -

          attaching current patch.

          Show
          Brock Noland added a comment - attaching current patch.
          Hide
          jiraposter@reviews.apache.org added a comment -

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

          Review request for Flume.

          Summary
          -------

          Catches, logs, and sleeps when a general exception occurs. This is similar to PollingSourceRunner.

          This addresses bug FLUME-984.
          https://issues.apache.org/jira/browse/FLUME-984

          Diffs


          flume-ng-core/src/main/java/org/apache/flume/SinkRunner.java 9e46d87

          Diff: https://reviews.apache.org/r/3980/diff

          Testing
          -------

          All unit tests pass.

          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/3980/ ----------------------------------------------------------- Review request for Flume. Summary ------- Catches, logs, and sleeps when a general exception occurs. This is similar to PollingSourceRunner. This addresses bug FLUME-984 . https://issues.apache.org/jira/browse/FLUME-984 Diffs flume-ng-core/src/main/java/org/apache/flume/SinkRunner.java 9e46d87 Diff: https://reviews.apache.org/r/3980/diff Testing ------- All unit tests pass. 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