HBase
  1. HBase
  2. HBASE-5796

Fix our abuse of IOE: see http://blog.tsunanet.net/2012/04/apache-hadoop-abuse-ioexception.html

    Details

    • Type: Task Task
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None

      Description

      Lets make more context particular exceptions rather than throw IOEs everywhere. See Benoît's rant: http://blog.tsunanet.net/2012/04/apache-hadoop-abuse-ioexception.html

        Issue Links

          Activity

          Hide
          Lars Hofhansl added a comment -

          Could be as simple as a better hierarchy of classes extending IOException (like DoNotRetryException).

          Show
          Lars Hofhansl added a comment - Could be as simple as a better hierarchy of classes extending IOException (like DoNotRetryException).
          Hide
          Benoit Sigoure added a comment -

          I propose the following, as a temporary stopgap solution:

          1. Create a new class that inherits from IOException, call it HBaseException.
          2. Change all HBase exceptions to inherit from them. Replace all bare new IOException with new HBaseException.
          3. Classify all exceptions to be under two categories: what can be potentially retried, and what cannot (this partially exists today under DoNotRetryException as Lars points out).
          4. At this point, nothing should be a bare HBaseException, and nothing should be creating bare {{IOException}}s either.
          5. Update all APIs to specify exceptions thrown with a type as specific as possible.

          I believe most (if not all) of the steps above can be done while preserving API compatibility (and maybe even ABI compatibility).

          This would be a good first pass to partially clean up some of the mess we have today. Also, because Hadoop is even worse than HBase, and because it's changing slowly, your code will still be crippled with IOException from Hadoop. I'd recommend wrapping them all in a HadoopIOException, which would inherit from HBaseException.

          Ultimately, I would strongly encourage you to move away from IOException altogether. Not only because it's the wrong exception to use in most cases where it's used in Hadoop/HBase, but also because it's a checked exception. As I wrote in my blog post, checked exceptions are like communism: they sound like a good idea, but they horribly fail in practice. Here's a short but good article that conveys well two of the biggest show-stoppers with checked exceptions: http://www.artima.com/intv/handcuffsP.html – I'm so happy that Scala didn't pick this up from Java. Checked exceptions almost always wind up defeating their own purpose. I'm sure you guys have written enough code in big enough code bases to experience this too.

          Show
          Benoit Sigoure added a comment - I propose the following, as a temporary stopgap solution: Create a new class that inherits from IOException , call it HBaseException . Change all HBase exceptions to inherit from them. Replace all bare new IOException with new HBaseException . Classify all exceptions to be under two categories: what can be potentially retried, and what cannot (this partially exists today under DoNotRetryException as Lars points out). At this point, nothing should be a bare HBaseException , and nothing should be creating bare {{IOException}}s either. Update all APIs to specify exceptions thrown with a type as specific as possible. I believe most (if not all) of the steps above can be done while preserving API compatibility (and maybe even ABI compatibility). This would be a good first pass to partially clean up some of the mess we have today. Also, because Hadoop is even worse than HBase, and because it's changing slowly, your code will still be crippled with IOException from Hadoop. I'd recommend wrapping them all in a HadoopIOException , which would inherit from HBaseException . Ultimately, I would strongly encourage you to move away from IOException altogether. Not only because it's the wrong exception to use in most cases where it's used in Hadoop/HBase, but also because it's a checked exception. As I wrote in my blog post, checked exceptions are like communism: they sound like a good idea, but they horribly fail in practice. Here's a short but good article that conveys well two of the biggest show-stoppers with checked exceptions: http://www.artima.com/intv/handcuffsP.html – I'm so happy that Scala didn't pick this up from Java. Checked exceptions almost always wind up defeating their own purpose. I'm sure you guys have written enough code in big enough code bases to experience this too.
          Hide
          Ted Yu added a comment -

          all in a HadoopIOException, which would inherit from HBaseException.

          I am afraid that some people would not feel comfortable with the above inheritance.

          Show
          Ted Yu added a comment - all in a HadoopIOException , which would inherit from HBaseException . I am afraid that some people would not feel comfortable with the above inheritance.
          Hide
          Benoit Sigoure added a comment -

          Just to be clear: the wrapping of Hadoop's IOException would be for a first pass, to make sure everything HBase throws inherits from an HBaseException. Ideally it would be better to pick a more specific type of exception, but I expect this will be difficult in various places where Hadoop suffers from the same problem and HBase itself has little to no visibility into what the exception from Hadoop really means.

          Also I'd recommend to actually use HBaseIOException as a base class name, and keep the name HBaseException for when you guys switch to unchecked exceptions, if you're willing to consider going down that route eventually (that'd require a separate JIRA issue).

          Since HBase 0.96 is the "singularity", it's a good opportunity to break API compatibility. In this case, every release up to and including 0.94.x would have HBaseIOException as the parent class, and in 0.96 the only thing you need to change is to make everything inherit from HBaseException (which itself would inherit from RuntimeException).

          Show
          Benoit Sigoure added a comment - Just to be clear: the wrapping of Hadoop's IOException would be for a first pass, to make sure everything HBase throws inherits from an HBaseException . Ideally it would be better to pick a more specific type of exception, but I expect this will be difficult in various places where Hadoop suffers from the same problem and HBase itself has little to no visibility into what the exception from Hadoop really means. Also I'd recommend to actually use HBaseIOException as a base class name, and keep the name HBaseException for when you guys switch to unchecked exceptions, if you're willing to consider going down that route eventually (that'd require a separate JIRA issue). Since HBase 0.96 is the "singularity", it's a good opportunity to break API compatibility. In this case, every release up to and including 0.94.x would have HBaseIOException as the parent class, and in 0.96 the only thing you need to change is to make everything inherit from HBaseException (which itself would inherit from RuntimeException ).
          Hide
          Jonathan Hsieh added a comment -

          HBaseException.java and DeserializationException.java snuck in with HBASE-5336

          I'm working on another patch (HBASE-6586) and adding am considering adding an HBaseIOException that is a subclass of IOException. This would allow for compatibility with older versions for the time being and would allow the code to be more readable if used properly.

          Show
          Jonathan Hsieh added a comment - HBaseException.java and DeserializationException.java snuck in with HBASE-5336 I'm working on another patch ( HBASE-6586 ) and adding am considering adding an HBaseIOException that is a subclass of IOException. This would allow for compatibility with older versions for the time being and would allow the code to be more readable if used properly.

            People

            • Assignee:
              Unassigned
              Reporter:
              stack
            • Votes:
              1 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:

                Development