Uploaded image for project: 'Samza'
  1. Samza
  2. SAMZA-1077

SamzaContainer should catch all Throwables instead of only exceptions

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.12.0
    • Component/s: None
    • Labels:
      None

      Description

      Here's the snippet from the startup sequence of the SamzaContainer.

      SamzaContainer.scala
            startProducers
            startTask
            startConsumers
            startSecurityManger
      
            info("Entering run loop.")
            addShutdownHook
            runLoop.run
          } catch {
            case e: Exception =>
              error("Caught exception in process loop.", e)
              throw e
          } finally {
            info("Shutting down.")
      
            shutdownConsumers
            shutdownTask
            shutdownStores
      

      In this case, the catch block merely catches Exceptions and not Errors.

      Consider the following sequence:
      1. `NoSuchMethodError` thrown in the startup sequence.
      2. The error is not caught in the catch block (hence, not logged) and the control reaches the finally block.
      3. The finally block throws an exception in one of the shutdown methods.
      4. The thrown exception completely masks the underlying NoSuchMethodError.

      Catching all Throwables will atleast help log the error.

      Ideally, finally block should be idempotent and not throw exceptions (but, that's for a different ticket since it's larger in scope)

        Issue Links

          Activity

          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user vjagadish opened a pull request:

          https://github.com/apache/samza/pull/40

          SAMZA-1077: Catch throwables in SamzaContainer

          Here's the snippet from the startup sequence of the SamzaContainer.

          SamzaContainer.scala
          ```
          startProducers
          startTask
          startConsumers
          startSecurityManger

          info("Entering run loop.")
          addShutdownHook
          runLoop.run
          } catch

          { case e: Exception => error("Caught exception in process loop.", e) throw e }

          finally {
          info("Shutting down.")

          shutdownConsumers
          shutdownTask
          shutdownStores
          ```
          In this case, the catch block should catch all Throwables instead of merely catching Exceptions. This will cause errors like `NoSuchMethodErrors` to be masked. Furthermore, the error can get lost completely if the finally block throws an exception during any of the shutdown calls.
          Catching all Throwables will prevent this.

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/vjagadish1989/samza samza-1077

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/samza/pull/40.patch

          To close this pull request, make a commit to your master/trunk branch
          with (at least) the following in the commit message:

          This closes #40


          commit 6ad9bd90a74331397c45c91d20583e8a0a8b27cb
          Author: vjagadish1989 <jvenkatr@linkedin.com>
          Date: 2017-01-06T19:27:26Z

          SAMZA-1077: Catch throwables in SamzaContainer


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user vjagadish opened a pull request: https://github.com/apache/samza/pull/40 SAMZA-1077 : Catch throwables in SamzaContainer Here's the snippet from the startup sequence of the SamzaContainer. SamzaContainer.scala ``` startProducers startTask startConsumers startSecurityManger info("Entering run loop.") addShutdownHook runLoop.run } catch { case e: Exception => error("Caught exception in process loop.", e) throw e } finally { info("Shutting down.") shutdownConsumers shutdownTask shutdownStores ``` In this case, the catch block should catch all Throwables instead of merely catching Exceptions. This will cause errors like `NoSuchMethodErrors` to be masked. Furthermore, the error can get lost completely if the finally block throws an exception during any of the shutdown calls. Catching all Throwables will prevent this. You can merge this pull request into a Git repository by running: $ git pull https://github.com/vjagadish1989/samza samza-1077 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/samza/pull/40.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #40 commit 6ad9bd90a74331397c45c91d20583e8a0a8b27cb Author: vjagadish1989 <jvenkatr@linkedin.com> Date: 2017-01-06T19:27:26Z SAMZA-1077 : Catch throwables in SamzaContainer
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user vjagadish closed the pull request at:

          https://github.com/apache/samza/pull/40

          Show
          githubbot ASF GitHub Bot added a comment - Github user vjagadish closed the pull request at: https://github.com/apache/samza/pull/40
          Hide
          jagadish1989@gmail.com Jagadish added a comment -

          Committed to master. Thanks Jake Maes for the review.

          Show
          jagadish1989@gmail.com Jagadish added a comment - Committed to master. Thanks Jake Maes for the review.

            People

            • Assignee:
              jagadish1989@gmail.com Jagadish
              Reporter:
              jagadish1989@gmail.com Jagadish
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development