Solr
  1. Solr
  2. SOLR-6002

Fix a couple of ugly issues around SolrIndexWriter close and rollback as well as how SolrIndexWriter manages it's ref counted directory instance.

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 4.9, 6.0
    • Component/s: None
    • Labels:
      None

      Description

      We tried jumping all kinds of hoops (unsuccessfully anyway) to really really close an IndexWriter in the past, but Lucene wouldn't let you. Now that is fixed in Lucene and we should update our close/rollback code.

      1. SOLR-6002.patch
        10 kB
        Mark Miller
      2. SOLR-6002.patch
        9 kB
        Mark Miller
      3. SOLR-6002.patch
        8 kB
        Mark Miller

        Issue Links

          Activity

          Hide
          Mark Miller added a comment -

          Just found a bug hidden in this - SolrIndexWriter should not release it's directory in close because it's managed externally and passed in.

          Show
          Mark Miller added a comment - Just found a bug hidden in this - SolrIndexWriter should not release it's directory in close because it's managed externally and passed in.
          Hide
          Mark Miller added a comment -

          Hmm...no not quiet that, but something is off...

          Show
          Mark Miller added a comment - Hmm...no not quiet that, but something is off...
          Hide
          Mark Miller added a comment -

          First pass on a patch. Much, much better.

          Show
          Mark Miller added a comment - First pass on a patch. Much, much better.
          Hide
          Mark Miller added a comment -

          I'll commit this shortly.

          Show
          Mark Miller added a comment - I'll commit this shortly.
          Hide
          Mark Miller added a comment -

          New, better patch.

          An issue we had previously is that close may or may not have called rollback and perhaps even the reverse. It seems now that close always calls rollback, but we shouldn't really count on that so I have changed things to assume that either may or may not call the other and SolrIndexWriter will still properly clean up.

          Show
          Mark Miller added a comment - New, better patch. An issue we had previously is that close may or may not have called rollback and perhaps even the reverse. It seems now that close always calls rollback, but we shouldn't really count on that so I have changed things to assume that either may or may not call the other and SolrIndexWriter will still properly clean up.
          Hide
          ASF subversion and git services added a comment -

          Commit 1589294 from Mark Miller in branch 'dev/trunk'
          [ https://svn.apache.org/r1589294 ]

          SOLR-6002: Fix a couple of ugly issues around SolrIndexWriter close and rollback as well as how SolrIndexWriter manages it's ref counted directory instance.

          Show
          ASF subversion and git services added a comment - Commit 1589294 from Mark Miller in branch 'dev/trunk' [ https://svn.apache.org/r1589294 ] SOLR-6002 : Fix a couple of ugly issues around SolrIndexWriter close and rollback as well as how SolrIndexWriter manages it's ref counted directory instance.
          Hide
          ASF subversion and git services added a comment -

          Commit 1589298 from Mark Miller in branch 'dev/branches/branch_4x'
          [ https://svn.apache.org/r1589298 ]

          SOLR-6002: Fix a couple of ugly issues around SolrIndexWriter close and rollback as well as how SolrIndexWriter manages it's ref counted directory instance.

          Show
          ASF subversion and git services added a comment - Commit 1589298 from Mark Miller in branch 'dev/branches/branch_4x' [ https://svn.apache.org/r1589298 ] SOLR-6002 : Fix a couple of ugly issues around SolrIndexWriter close and rollback as well as how SolrIndexWriter manages it's ref counted directory instance.
          Hide
          Gregory Chanan added a comment -

          1) I don't understand the need for the CLOSE_LOCK, can you explain?

          2) In DirectUpdateHandler2 why the change from writer.shutdown() to writer.close()? All the other changes go in the other direction, e.g. close -> shutdown.

          Show
          Gregory Chanan added a comment - 1) I don't understand the need for the CLOSE_LOCK, can you explain? 2) In DirectUpdateHandler2 why the change from writer.shutdown() to writer.close()? All the other changes go in the other direction, e.g. close -> shutdown.
          Hide
          Mark Miller added a comment -

          Thanks for the review!

          1) I don't understand the need for the CLOSE_LOCK, can you explain?

          Just so that only one thread will actually do the close logic. You can close an IndexWriter with either rollback or close and one of those might internally call the other - so the CLOSE_LOCK just makes sure the SolrIndexWriter close logic happens once. We don't use the this lock just so that we don't end up conflicting with other uses.

          2) In DirectUpdateHandler2 why the change from writer.shutdown() to writer.close()? All the other changes go in the other direction, e.g. close -> shutdown.

          Because shutdown will wait for merges and commit, etc and DirectUpdateHandler2 actually does that right above - optionally - it was actually a small bug that someone changed it to shutdown (that's just on branch 5x though). We didn't want to commit in all cases like shutdown was doing.

          I do see a problem though - we need to call waitForMerges before we close.

          Show
          Mark Miller added a comment - Thanks for the review! 1) I don't understand the need for the CLOSE_LOCK, can you explain? Just so that only one thread will actually do the close logic. You can close an IndexWriter with either rollback or close and one of those might internally call the other - so the CLOSE_LOCK just makes sure the SolrIndexWriter close logic happens once. We don't use the this lock just so that we don't end up conflicting with other uses. 2) In DirectUpdateHandler2 why the change from writer.shutdown() to writer.close()? All the other changes go in the other direction, e.g. close -> shutdown. Because shutdown will wait for merges and commit, etc and DirectUpdateHandler2 actually does that right above - optionally - it was actually a small bug that someone changed it to shutdown (that's just on branch 5x though). We didn't want to commit in all cases like shutdown was doing. I do see a problem though - we need to call waitForMerges before we close.
          Hide
          Mark Miller added a comment -

          I do see a problem though - we need to call waitForMerges before we close.

          Though I guess that fine points of that are up for debate. Waiting for merges can take a long time - perhaps we should just bail on them on shutdown - you don't lose data from that. Or perhaps we should wait when we commit on shutdown and not when we don't.

          I almost favor not waiting in either case - when I want to shutdown, I want it to be fairly prompt. Perhaps it should be a configuration and default to off. I don't want to tie all that into this issue though. We should probably just wait for merges for now as I think it's been done for a long time.

          Show
          Mark Miller added a comment - I do see a problem though - we need to call waitForMerges before we close. Though I guess that fine points of that are up for debate. Waiting for merges can take a long time - perhaps we should just bail on them on shutdown - you don't lose data from that. Or perhaps we should wait when we commit on shutdown and not when we don't. I almost favor not waiting in either case - when I want to shutdown, I want it to be fairly prompt. Perhaps it should be a configuration and default to off. I don't want to tie all that into this issue though. We should probably just wait for merges for now as I think it's been done for a long time.
          Hide
          Gregory Chanan added a comment -

          Just so that only one thread will actually do the close logic. You can close an IndexWriter with either rollback or close and one of those might internally call the other - so the CLOSE_LOCK just makes sure the SolrIndexWriter close logic happens once. We don't use the this lock just so that we don't end up conflicting with other uses.

          Why is the CLOSE_LOCK static though?

          Though I guess that fine points of that are up for debate. Waiting for merges can take a long time - perhaps we should just bail on them on shutdown - you don't lose data from that. Or perhaps we should wait when we commit on shutdown and not when we don't.

          I almost favor not waiting in either case - when I want to shutdown, I want it to be fairly prompt. Perhaps it should be a configuration and default to off. I don't want to tie all that into this issue though. We should probably just wait for merges for now as I think it's been done for a long time.

          Waiting for now with a config seems reasonable, but I don't have a strong opinion.

          Show
          Gregory Chanan added a comment - Just so that only one thread will actually do the close logic. You can close an IndexWriter with either rollback or close and one of those might internally call the other - so the CLOSE_LOCK just makes sure the SolrIndexWriter close logic happens once. We don't use the this lock just so that we don't end up conflicting with other uses. Why is the CLOSE_LOCK static though? Though I guess that fine points of that are up for debate. Waiting for merges can take a long time - perhaps we should just bail on them on shutdown - you don't lose data from that. Or perhaps we should wait when we commit on shutdown and not when we don't. I almost favor not waiting in either case - when I want to shutdown, I want it to be fairly prompt. Perhaps it should be a configuration and default to off. I don't want to tie all that into this issue though. We should probably just wait for merges for now as I think it's been done for a long time. Waiting for now with a config seems reasonable, but I don't have a strong opinion.
          Hide
          Mark Miller added a comment -

          Why is the CLOSE_LOCK static though?

          Whoops - just fell in line with the final statics around it I guess - no need for a global lock though.

          Show
          Mark Miller added a comment - Why is the CLOSE_LOCK static though? Whoops - just fell in line with the final statics around it I guess - no need for a global lock though.
          Hide
          ASF subversion and git services added a comment -

          Commit 1589327 from Mark Miller in branch 'dev/trunk'
          [ https://svn.apache.org/r1589327 ]

          SOLR-6002: CLOSE_LOCK does not need to be static, waitForMerges when DirectUpdateHandler2 closes the IndexWriter, add Greg to CHANGES entry.

          Show
          ASF subversion and git services added a comment - Commit 1589327 from Mark Miller in branch 'dev/trunk' [ https://svn.apache.org/r1589327 ] SOLR-6002 : CLOSE_LOCK does not need to be static, waitForMerges when DirectUpdateHandler2 closes the IndexWriter, add Greg to CHANGES entry.
          Hide
          Gregory Chanan added a comment -

          +1 lgtm

          Show
          Gregory Chanan added a comment - +1 lgtm
          Hide
          ASF subversion and git services added a comment -

          Commit 1589328 from Mark Miller in branch 'dev/branches/branch_4x'
          [ https://svn.apache.org/r1589328 ]

          SOLR-6002: CLOSE_LOCK does not need to be static, waitForMerges when DirectUpdateHandler2 closes the IndexWriter, add Greg to CHANGES entry.

          Show
          ASF subversion and git services added a comment - Commit 1589328 from Mark Miller in branch 'dev/branches/branch_4x' [ https://svn.apache.org/r1589328 ] SOLR-6002 : CLOSE_LOCK does not need to be static, waitForMerges when DirectUpdateHandler2 closes the IndexWriter, add Greg to CHANGES entry.

            People

            • Assignee:
              Mark Miller
              Reporter:
              Mark Miller
            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development