Lucene - Core
  1. Lucene - Core
  2. LUCENE-6217

IndexWriter should make it clear when tragedy strikes

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 5.1, 6.0
    • Component/s: None
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      If you hit an exception at a "bad time" e.g. when writing files for a newly flushed segment, IndexWriter declares it a tragedy and secretly closes itself as a side effect of the exception.

      Subsequent operations will throw an ACE with the exception that caused the tragedy as its cause.

      This requires messy code, if you want to know when this happened to you, since the first exception doesn't make it clear that it was "tragic".

      I think we should make it easier to know when this happens?

      Maybe we could instead throw a new exception (IWClosedByTragedy or something), or maybe we add a getter (.getTragicException) to IW?

      1. LUCENE-6217.patch
        4 kB
        Michael McCandless

        Activity

        Hide
        Ryan Ernst added a comment -

        Maybe we could instead throw a new exception (IWClosedByTragedy or something)

        +1

        Show
        Ryan Ernst added a comment - Maybe we could instead throw a new exception (IWClosedByTragedy or something) +1
        Hide
        Robert Muir added a comment -

        I would rather have the getter than confusingly change exception types.

        we shouldn't make this more complicated than it already is: you cannot recover from this situation anyway.

        Show
        Robert Muir added a comment - I would rather have the getter than confusingly change exception types. we shouldn't make this more complicated than it already is: you cannot recover from this situation anyway.
        Hide
        Simon Willnauer added a comment -

        I just wanna point out the consequences if you provide this as a getter rather than an exception:

          try {
            iw.addDocument(doc);
          } catch (TragicEventException ex) {
            // notify others that all bets are off
          } 
        

        vs.

          try {
            iw.addDocument(doc);
          } catch (Throwable ex) {
            Throwable tragic = iw.getTragicEventException();
            if (tragic != null) {
              // notify others that all bets are off
            }
            throw ex; 
          } 
        

        99% of the users won't catch this but would be happy to see it right away what happened. If you need to catch it ie. if you care to notify the dedicated exception is way nicer and more intuitive too. I'd vote for the exception and I don't see how this makes things more complicated, I am sure you have something in mind can you elaborate on that?

        Show
        Simon Willnauer added a comment - I just wanna point out the consequences if you provide this as a getter rather than an exception: try { iw.addDocument(doc); } catch (TragicEventException ex) { // notify others that all bets are off } vs. try { iw.addDocument(doc); } catch (Throwable ex) { Throwable tragic = iw.getTragicEventException(); if (tragic != null ) { // notify others that all bets are off } throw ex; } 99% of the users won't catch this but would be happy to see it right away what happened. If you need to catch it ie. if you care to notify the dedicated exception is way nicer and more intuitive too. I'd vote for the exception and I don't see how this makes things more complicated, I am sure you have something in mind can you elaborate on that?
        Hide
        Robert Muir added a comment -

        The way i see it, 100% of users wont catch this. The reason is you cannot recover from it. If you cannot flush segments or some other error like this, usually manual intervention is necessary. Its not something you can recover from.

        So, I dont think we need to make big changes to indexwriter or overengineer an API for it. Changing all exception types to be a fake one definitely counts as that to me.

        Show
        Robert Muir added a comment - The way i see it, 100% of users wont catch this. The reason is you cannot recover from it. If you cannot flush segments or some other error like this, usually manual intervention is necessary. Its not something you can recover from. So, I dont think we need to make big changes to indexwriter or overengineer an API for it. Changing all exception types to be a fake one definitely counts as that to me.
        Hide
        Simon Willnauer added a comment -

        The way i see it, 100% of users wont catch this.

        I need to catch it!

        Show
        Simon Willnauer added a comment - The way i see it, 100% of users wont catch this. I need to catch it!
        Hide
        Simon Willnauer added a comment -

        I think being able to check indexWriter.isOpen() would solve most of the issues. If I get indexWriter.getTragicException() I am good with it.

        Show
        Simon Willnauer added a comment - I think being able to check indexWriter.isOpen() would solve most of the issues. If I get indexWriter.getTragicException() I am good with it.
        Hide
        Michael McCandless added a comment -

        I'll add a getter...

        Show
        Michael McCandless added a comment - I'll add a getter...
        Hide
        Michael McCandless added a comment -

        Simple patch, adding getTragicException and isOpen to IW, and I fixed a couple tests to use these methods.

        Show
        Michael McCandless added a comment - Simple patch, adding getTragicException and isOpen to IW, and I fixed a couple tests to use these methods.
        Hide
        Robert Muir added a comment -

        +1 for these new methods and for the bonus test improvements, thats a nice solution.

        Show
        Robert Muir added a comment - +1 for these new methods and for the bonus test improvements, thats a nice solution.
        Hide
        Ryan Ernst added a comment -

        I do like the test changes. +1

        Show
        Ryan Ernst added a comment - I do like the test changes. +1
        Hide
        ASF subversion and git services added a comment -

        Commit 1657398 from Michael McCandless in branch 'dev/trunk'
        [ https://svn.apache.org/r1657398 ]

        LUCENE-6217: add IW.isOpen and IW.getTragicException

        Show
        ASF subversion and git services added a comment - Commit 1657398 from Michael McCandless in branch 'dev/trunk' [ https://svn.apache.org/r1657398 ] LUCENE-6217 : add IW.isOpen and IW.getTragicException
        Hide
        ASF subversion and git services added a comment -

        Commit 1657399 from Michael McCandless in branch 'dev/branches/branch_5x'
        [ https://svn.apache.org/r1657399 ]

        LUCENE-6217: add IW.isOpen and IW.getTragicException

        Show
        ASF subversion and git services added a comment - Commit 1657399 from Michael McCandless in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1657399 ] LUCENE-6217 : add IW.isOpen and IW.getTragicException
        Hide
        Timothy Potter added a comment -

        Bulk close after 5.1 release

        Show
        Timothy Potter added a comment - Bulk close after 5.1 release

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development