Uploaded image for project: 'Lucene - Core'
  1. Lucene - Core
  2. LUCENE-793

Javadocs should explain possible causes for IOExceptions

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.2
    • Component/s: general/javadocs
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      Most methods in Lucene reserve the right to throw an IOException. This can occur for nearly all methods from low level problems like wrong permissions, transient IO errors, bad hard drive or corrupted file system, corrupted index, etc, but for some methods there are also more interesting causes that we should try to document.

      Spinoff of this thread:

      http://www.gossamer-threads.com/lists/lucene/java-user/44929

      1. LUCENE-793.patch
        90 kB
        Michael McCandless

        Activity

        Hide
        cutting Doug Cutting added a comment -

        Should we add more precise exceptions for these cases, and add them to the 'throws' for these methods? If we make these subclasses of IOException then this would be back-compatible.

        Show
        cutting Doug Cutting added a comment - Should we add more precise exceptions for these cases, and add them to the 'throws' for these methods? If we make these subclasses of IOException then this would be back-compatible.
        Hide
        mikemccand Michael McCandless added a comment -

        Good idea! I will take that approach.

        Show
        mikemccand Michael McCandless added a comment - Good idea! I will take that approach.
        Hide
        mikemccand Michael McCandless added a comment -

        OK I created 3 subclasses of IOException:

        org.apache.lucene.store.LockObtainFailedException
        org.apache.lucene.index.CorruptIndexException
        org.apache.lucene.index.StaleReaderException

        and then changed most places where we throw a newly created
        IOException to instead throw one of these. We still throw an
        IOException for low-level errors (eg "can't read directory" or "can't
        find segments_N file").

        I updated javadocs to reflect these changes.

        Other small changes:

        • Fixed IndexReader to throw IOException if you try to
          delete/setNorm/undeleteAll after the reader is closed (it wasn't
          previously). Added unit test for this.
        • Fixed one respelling in private method (aquireWriteLock ->
          acquireWriteLock)
        • Changed some places where we were throwing IllegalStateException
          to throw CorruptIndexException instead
        • Fixed some javadoc warnings
        Show
        mikemccand Michael McCandless added a comment - OK I created 3 subclasses of IOException: org.apache.lucene.store.LockObtainFailedException org.apache.lucene.index.CorruptIndexException org.apache.lucene.index.StaleReaderException and then changed most places where we throw a newly created IOException to instead throw one of these. We still throw an IOException for low-level errors (eg "can't read directory" or "can't find segments_N file"). I updated javadocs to reflect these changes. Other small changes: Fixed IndexReader to throw IOException if you try to delete/setNorm/undeleteAll after the reader is closed (it wasn't previously). Added unit test for this. Fixed one respelling in private method (aquireWriteLock -> acquireWriteLock) Changed some places where we were throwing IllegalStateException to throw CorruptIndexException instead Fixed some javadoc warnings
        Hide
        hossman Hoss Man added a comment -

        > * Changed some places where we were throwing IllegalStateException
        > to throw CorruptIndexException instead

        shouldn't this be considered a major API change? people previously catching IllegalStateException to try and deal with this differently from an IOException will now be caching the "CorruptIndexException" as a subclass of IOException.

        I haven't read the details of the patch, but perhaps CorruptIndexException should be a subclass of IllegalStateException to be backwards compatible?

        Show
        hossman Hoss Man added a comment - > * Changed some places where we were throwing IllegalStateException > to throw CorruptIndexException instead shouldn't this be considered a major API change? people previously catching IllegalStateException to try and deal with this differently from an IOException will now be caching the "CorruptIndexException" as a subclass of IOException. I haven't read the details of the patch, but perhaps CorruptIndexException should be a subclass of IllegalStateException to be backwards compatible?
        Hide
        mikemccand Michael McCandless added a comment -

        You're right, technically this is an API change.

        The only cases I changed were the ones stemming from LUCENE-140: the
        original "docs out of order" corruption plus 2 other consisteny checks
        I had added as part of the fix for LUCENE-140.

        (There are other places where Lucene throws a IllegalStateException
        that I did not change.)

        I think it's extremely unlikely users are relying on the
        IllegalStateException specifically (ie catching it explicitly and
        doing something about it)?

        EG these particular cases were never listed in the "throws"
        (IllegalStateException is unchecked). And if you hit this exception
        your index truly is corrupt.

        Since these really are cases of severe index corruption I thought it
        best to throw CorruptIndexException instead?

        Show
        mikemccand Michael McCandless added a comment - You're right, technically this is an API change. The only cases I changed were the ones stemming from LUCENE-140 : the original "docs out of order" corruption plus 2 other consisteny checks I had added as part of the fix for LUCENE-140 . (There are other places where Lucene throws a IllegalStateException that I did not change.) I think it's extremely unlikely users are relying on the IllegalStateException specifically (ie catching it explicitly and doing something about it)? EG these particular cases were never listed in the "throws" (IllegalStateException is unchecked). And if you hit this exception your index truly is corrupt. Since these really are cases of severe index corruption I thought it best to throw CorruptIndexException instead?
        Hide
        hossman Hoss Man added a comment -

        > Since these really are cases of severe index corruption I thought it
        > best to throw CorruptIndexException instead?

        no disagreement, i'm just not clear on why "CorruptIndexException" (which is a completley new Exception type created by your patch correct?) should be a subclass of IOException instead of IllegalStateException.

        Show
        hossman Hoss Man added a comment - > Since these really are cases of severe index corruption I thought it > best to throw CorruptIndexException instead? no disagreement, i'm just not clear on why "CorruptIndexException" (which is a completley new Exception type created by your patch correct?) should be a subclass of IOException instead of IllegalStateException.
        Hide
        mikemccand Michael McCandless added a comment -

        Ahh, OK, good question. Yes this a new exception created by this patch.

        There are quite a few places (8 actually) where we previously threw an
        IOException and I've now changed to a CorruptIndexException. Also
        since IOException is checked, there are presumably many catch clauses
        out there that would at least catch (yet probably not handle) these
        corruption cases now.

        All of these cases, plus the IllegalStateException cases, should be
        exceptionally rare, but I think it's "more" backwards compatible to
        leave the base class of the new CorruptIndexException as IOException?

        Show
        mikemccand Michael McCandless added a comment - Ahh, OK, good question. Yes this a new exception created by this patch. There are quite a few places (8 actually) where we previously threw an IOException and I've now changed to a CorruptIndexException. Also since IOException is checked, there are presumably many catch clauses out there that would at least catch (yet probably not handle) these corruption cases now. All of these cases, plus the IllegalStateException cases, should be exceptionally rare, but I think it's "more" backwards compatible to leave the base class of the new CorruptIndexException as IOException?
        Hide
        hossman Hoss Man added a comment -

        Hmmm... well my paranoia says that if we are trying to make the Exceptions more explicit, then any place we currently "throw Foo" we should "throw Bar" where "Foo.class.isAssignable(Bar.class)" ... but if we currently mix/match IOException/IllegalStateException in cases of corruption then I guess it makes sense to use CorruptIndexException extends IOException.

        (the only alternative i can think of would be a CorruptIndexIOException and a CorruptIndexStateException .. but you're probably right .. i don't very many people are worried about catching the IllegalStateException and doing anyhitng special that they aren't already doing when they catch the IOException.

        Show
        hossman Hoss Man added a comment - Hmmm... well my paranoia says that if we are trying to make the Exceptions more explicit, then any place we currently "throw Foo" we should "throw Bar" where "Foo.class.isAssignable(Bar.class)" ... but if we currently mix/match IOException/IllegalStateException in cases of corruption then I guess it makes sense to use CorruptIndexException extends IOException. (the only alternative i can think of would be a CorruptIndexIOException and a CorruptIndexStateException .. but you're probably right .. i don't very many people are worried about catching the IllegalStateException and doing anyhitng special that they aren't already doing when they catch the IOException.
        Hide
        mikemccand Michael McCandless added a comment -

        OK, I just committed this. I stuck with the original approach (basing CorruptIndexException on IOException).

        Show
        mikemccand Michael McCandless added a comment - OK, I just committed this. I stuck with the original approach (basing CorruptIndexException on IOException).

          People

          • Assignee:
            mikemccand Michael McCandless
            Reporter:
            mikemccand Michael McCandless
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development