Solr
  1. Solr
  2. SOLR-1155

Change DirectUpdateHandler2 to allow concurrent adds during an autocommit

    Details

    • Type: Improvement Improvement
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: 1.3, 1.4
    • Fix Version/s: 4.8
    • Component/s: search
    • Labels:
      None

      Description

      Currently DirectUpdateHandler2 will block adds during a commit, and it seems to be possible with recent changes to Lucene to allow them to run concurrently.

      See: http://www.nabble.com/Autocommit-blocking-adds---AutoCommit-Speedup--td23435224.html

      1. SOLR-1155-trunk-rev834706.patch
        75 kB
        Jayson Minard
      2. SOLR-1155-release1.4-rev834789.patch
        75 kB
        Jayson Minard
      3. Solr-1155.patch
        51 kB
        Jayson Minard
      4. Solr-1155.patch
        53 kB
        Jayson Minard

        Issue Links

          Activity

          Hide
          Steve Rowe added a comment -

          Bulk move 4.4 issues to 4.5 and 5.0

          Show
          Steve Rowe added a comment - Bulk move 4.4 issues to 4.5 and 5.0
          Hide
          Hoss Man added a comment -

          Bulk of fixVersion=3.6 -> fixVersion=4.0 for issues that have no assignee and have not been updated recently.

          email notification suppressed to prevent mass-spam
          psuedo-unique token identifying these issues: hoss20120321nofix36

          Show
          Hoss Man added a comment - Bulk of fixVersion=3.6 -> fixVersion=4.0 for issues that have no assignee and have not been updated recently. email notification suppressed to prevent mass-spam psuedo-unique token identifying these issues: hoss20120321nofix36
          Hide
          Robert Muir added a comment -

          3.4 -> 3.5

          Show
          Robert Muir added a comment - 3.4 -> 3.5
          Hide
          Robert Muir added a comment -

          Bulk move 3.2 -> 3.3

          Show
          Robert Muir added a comment - Bulk move 3.2 -> 3.3
          Hide
          Jayson Minard added a comment -

          I'll look at updating this for 3.1 for those that need it on that release, and Mark's looks good for 4.x and beyond.

          Show
          Jayson Minard added a comment - I'll look at updating this for 3.1 for those that need it on that release, and Mark's looks good for 4.x and beyond.
          Hide
          Jayson Minard added a comment -

          Thank Yonik, I'll take a look at his to see if there was anything I learned that applies. This SOLR-1155 has been used in heavy production load and is very stable against 1.4 so maybe Mark will take a peek, I posted a note on the other issue as well.

          Show
          Jayson Minard added a comment - Thank Yonik, I'll take a look at his to see if there was anything I learned that applies. This SOLR-1155 has been used in heavy production load and is very stable against 1.4 so maybe Mark will take a peek, I posted a note on the other issue as well.
          Hide
          Yonik Seeley added a comment -

          There's absolutely a lot of interest in this feature.
          Have you checked out SOLR-2193? It seems like a dup of this issue, but Mark just uploaded a patch for trunk.

          Show
          Yonik Seeley added a comment - There's absolutely a lot of interest in this feature. Have you checked out SOLR-2193 ? It seems like a dup of this issue, but Mark just uploaded a patch for trunk.
          Hide
          Jayson Minard added a comment -

          Is there interest in me updating this for 3.1? It is a huge performance improvement over DirectUpdateHandler2 under heavy indexing load...

          Show
          Jayson Minard added a comment - Is there interest in me updating this for 3.1? It is a huge performance improvement over DirectUpdateHandler2 under heavy indexing load...
          Hide
          Hoss Man added a comment -

          Bulk updating 240 Solr issues to set the Fix Version to "next" per the process outlined in this email...

          http://mail-archives.apache.org/mod_mbox/lucene-dev/201005.mbox/%3Calpine.DEB.1.10.1005251052040.24672@radix.cryptio.net%3E

          Selection criteria was "Unresolved" with a Fix Version of 1.5, 1.6, 3.1, or 4.0. email notifications were suppressed.

          A unique token for finding these 240 issues in the future: hossversioncleanup20100527

          Show
          Hoss Man added a comment - Bulk updating 240 Solr issues to set the Fix Version to "next" per the process outlined in this email... http://mail-archives.apache.org/mod_mbox/lucene-dev/201005.mbox/%3Calpine.DEB.1.10.1005251052040.24672@radix.cryptio.net%3E Selection criteria was "Unresolved" with a Fix Version of 1.5, 1.6, 3.1, or 4.0. email notifications were suppressed. A unique token for finding these 240 issues in the future: hossversioncleanup20100527
          Hide
          Jayson Minard added a comment -

          Added patch for release version of 1.4 (but this is most likely identical to the last trunk patch anyway)

          Show
          Jayson Minard added a comment - Added patch for release version of 1.4 (but this is most likely identical to the last trunk patch anyway)
          Hide
          Jayson Minard added a comment -

          Updated patch to trunk rev 834706 and up to date with current DirectUpdateHandler2 functionality

          Show
          Jayson Minard added a comment - Updated patch to trunk rev 834706 and up to date with current DirectUpdateHandler2 functionality
          Hide
          Jayson Minard added a comment -

          Resolve TODO for commitWithin, and updated AutoCommitTrackerTest to validate the fix.

          Show
          Jayson Minard added a comment - Resolve TODO for commitWithin, and updated AutoCommitTrackerTest to validate the fix.
          Hide
          Jayson Minard added a comment -

          TODO: fix commitWithin, it isn't respected by the DirectUpdateHandler3

          Show
          Jayson Minard added a comment - TODO: fix commitWithin, it isn't respected by the DirectUpdateHandler3
          Hide
          Jayson Minard added a comment -

          Cleaning the patch so that RequestHandlerBase only has the required change and not the accidental reformatting.

          Show
          Jayson Minard added a comment - Cleaning the patch so that RequestHandlerBase only has the required change and not the accidental reformatting.
          Hide
          Jayson Minard added a comment -

          Updated unit test to compile (was missing method change from previous patch that changed an interface it implemented)

          Show
          Jayson Minard added a comment - Updated unit test to compile (was missing method change from previous patch that changed an interface it implemented)
          Hide
          Jayson Minard added a comment -

          Changed external commit interface to not have locking, but rather query if a commit/rollback/optimize is already running to accomplish the same goal. Also ensure no locking around waiting for searcher after commit or optimize.

          Show
          Jayson Minard added a comment - Changed external commit interface to not have locking, but rather query if a commit/rollback/optimize is already running to accomplish the same goal. Also ensure no locking around waiting for searcher after commit or optimize.
          Hide
          Jayson Minard added a comment -

          Change openWriter to do a check of null writer before locking to avoid artificial contention between threads due to ensuring a writer is open.

          Show
          Jayson Minard added a comment - Change openWriter to do a check of null writer before locking to avoid artificial contention between threads due to ensuring a writer is open.
          Hide
          Jayson Minard added a comment -

          Bug fixing, and added a new test that test just the AutoCommitTracker in isolation rather than through the DirectUpdateHandlerX

          Show
          Jayson Minard added a comment - Bug fixing, and added a new test that test just the AutoCommitTracker in isolation rather than through the DirectUpdateHandlerX
          Hide
          Jayson Minard added a comment -

          Knocked down a few TODO and cleaned up resetting of counts.

          Show
          Jayson Minard added a comment - Knocked down a few TODO and cleaned up resetting of counts.
          Hide
          Jayson Minard added a comment -

          minor change to rollback to only drop and re-open writer if there was one to begin wtih.

          Show
          Jayson Minard added a comment - minor change to rollback to only drop and re-open writer if there was one to begin wtih.
          Hide
          Jayson Minard added a comment -

          Refactored the AutoCommitTracker out to separate top-level class rather than inner class. Enhanced the interface used by unit tests and other classes to talk to the update handler so make this work. Interface is not final, just a temp answer to the decoupling.

          Show
          Jayson Minard added a comment - Refactored the AutoCommitTracker out to separate top-level class rather than inner class. Enhanced the interface used by unit tests and other classes to talk to the update handler so make this work. Interface is not final, just a temp answer to the decoupling.
          Hide
          Jayson Minard added a comment -

          move wait for searcher out of locks in optimize and commit

          Show
          Jayson Minard added a comment - move wait for searcher out of locks in optimize and commit
          Hide
          Jayson Minard added a comment -

          Does anyone know if the UpdateHandler.close() method is called when everything else is shut down and no transactions can come through? If so, I can remove the blocking added to prevent a writer from re-opening after close is called.

          Show
          Jayson Minard added a comment - Does anyone know if the UpdateHandler.close() method is called when everything else is shut down and no transactions can come through? If so, I can remove the blocking added to prevent a writer from re-opening after close is called.
          Hide
          Jayson Minard added a comment -

          New patch resolves the serialization of commit/optimize/close/rollback actions that should not be concurrent with each other. Also protect methods after close to keep other threads from accidentally re-opening writer.

          Show
          Jayson Minard added a comment - New patch resolves the serialization of commit/optimize/close/rollback actions that should not be concurrent with each other. Also protect methods after close to keep other threads from accidentally re-opening writer.
          Hide
          Jayson Minard added a comment -

          new TODO...

          Right now auto commit will eventually fire off a commit that might run into a manual commit or optimize and sit at the syncrhonization block, then fire off on the tail of the other action. I would rather it wait until those running commands clear and THEN evaluate if it needs to do a commit or not since its information it used to make the decision could be moot due to the other action that beat it in. So if a commit or optimize is running it will not evaluate for auto commit until they complete.

          Show
          Jayson Minard added a comment - new TODO... Right now auto commit will eventually fire off a commit that might run into a manual commit or optimize and sit at the syncrhonization block, then fire off on the tail of the other action. I would rather it wait until those running commands clear and THEN evaluate if it needs to do a commit or not since its information it used to make the decision could be moot due to the other action that beat it in. So if a commit or optimize is running it will not evaluate for auto commit until they complete.
          Hide
          Jayson Minard added a comment -

          Fixed some of the issue mentioned in the comment above. Split commit and optimize work out since optimize has a stronger need to lock and be consistent.

          Show
          Jayson Minard added a comment - Fixed some of the issue mentioned in the comment above. Split commit and optimize work out since optimize has a stronger need to lock and be consistent.
          Hide
          Jayson Minard added a comment -

          Things likely needing further work:

          • serialize commits? (two shouldn't happen at same time)
          • optimize should us iwManageWriter lock from optimize through call-back call, so break it away from commit which only needs iwUseWriter?) This also takes care of the contract that optimize callback is still on an optimized index.
          Show
          Jayson Minard added a comment - Things likely needing further work: serialize commits? (two shouldn't happen at same time) optimize should us iwManageWriter lock from optimize through call-back call, so break it away from commit which only needs iwUseWriter?) This also takes care of the contract that optimize callback is still on an optimized index.
          Hide
          Jayson Minard added a comment -

          All tests pass with the attached patch file and DirectUpdateHandler3 as the handler for each solrconfig variant in the test-files directory. Needs review for locking holes and some of the TODO comments answered still. And needs a lot of concurrent update testing.

          Show
          Jayson Minard added a comment - All tests pass with the attached patch file and DirectUpdateHandler3 as the handler for each solrconfig variant in the test-files directory. Needs review for locking holes and some of the TODO comments answered still. And needs a lot of concurrent update testing.
          Hide
          Jayson Minard added a comment -

          Updated file, and associated interface used to help DirectUpdateHandler2 and 3 co-exist with tests, SnapPuller and ReplicationHandler classes.

          A larger patch with all changes to tests, config files, and other items makeing DirectUpdateHandler3 the default (purely for running all of the unit tests using it) is attached as well.

          Patch includes fix for SOLR-1157 which got in the way of running unit tests.

          Show
          Jayson Minard added a comment - Updated file, and associated interface used to help DirectUpdateHandler2 and 3 co-exist with tests, SnapPuller and ReplicationHandler classes. A larger patch with all changes to tests, config files, and other items makeing DirectUpdateHandler3 the default (purely for running all of the unit tests using it) is attached as well. Patch includes fix for SOLR-1157 which got in the way of running unit tests.
          Hide
          Jayson Minard added a comment -

          Updated with different locking around the post commit/optimize handlers. There is a gap in time between commit and these calls, is that safe? left TODO's in the code at places of concern.

          Show
          Jayson Minard added a comment - Updated with different locking around the post commit/optimize handlers. There is a gap in time between commit and these calls, is that safe? left TODO's in the code at places of concern.
          Hide
          Jayson Minard added a comment -

          I created DirectUpdateHandler3 as a copy of DirectUpdateHandler2 and then did a pass across it rebuilding the locking, stats, and tracker. I am basically at the "IDE show no errors" stage and going to read it a few more times, and start testing. Feedback is welcome as I'm in the dark on a few areas of this code and I am working from a lot of assumptions. Some event placeholder methods remain while I think through the issues, will trim it back when done.

          Events to the auto commit tracker are within the locks to help avoid cases where we set the counts to 0 after other work has concurrently been done. The stats information has similar concerns and is not yet dealt with, so some stats might reset to 0 on commit when there was further work being done (so they should not go to 0).

          Show
          Jayson Minard added a comment - I created DirectUpdateHandler3 as a copy of DirectUpdateHandler2 and then did a pass across it rebuilding the locking, stats, and tracker. I am basically at the "IDE show no errors" stage and going to read it a few more times, and start testing. Feedback is welcome as I'm in the dark on a few areas of this code and I am working from a lot of assumptions. Some event placeholder methods remain while I think through the issues, will trim it back when done. Events to the auto commit tracker are within the locks to help avoid cases where we set the counts to 0 after other work has concurrently been done. The stats information has similar concerns and is not yet dealt with, so some stats might reset to 0 on commit when there was further work being done (so they should not go to 0).
          Hide
          Jayson Minard added a comment -

          Preparing a first patch, calling it DirectUpdateHandler3 as there is a lot of reshuffling and a new commit tracker implementation that is lighter-weight.

          Show
          Jayson Minard added a comment - Preparing a first patch, calling it DirectUpdateHandler3 as there is a lot of reshuffling and a new commit tracker implementation that is lighter-weight.
          Hide
          Jayson Minard added a comment -

          Thinking about pending counts as well. If adds are ongoing during a commit, there has to be a point in time where the counters clear the number of documents committed. Currently they are set to 0 which is safe since nothing was ongoing while the commit happened. Something to remember...

          Show
          Jayson Minard added a comment - Thinking about pending counts as well. If adds are ongoing during a commit, there has to be a point in time where the counters clear the number of documents committed. Currently they are set to 0 which is safe since nothing was ongoing while the commit happened. Something to remember...
          Hide
          Jayson Minard added a comment -

          Yonik mentioned: "We'd need to think about what type of synchronization may be needed
          for postCommit and postOptimize hooks too."

          Is the expectation for those callbacks that nothing will change until the callback completes? (i.e. a script might run a process that scans the index after optimize expecting it to remain optimized during the script execution) Or is that not the contract of those callbacks?

          Show
          Jayson Minard added a comment - Yonik mentioned: "We'd need to think about what type of synchronization may be needed for postCommit and postOptimize hooks too." Is the expectation for those callbacks that nothing will change until the callback completes? (i.e. a script might run a process that scans the index after optimize expecting it to remain optimized during the script execution) Or is that not the contract of those callbacks?
          Hide
          Jayson Minard added a comment - - edited

          This will involve a change to locking as seen in the thread mentioned above between add/delete/commit methods, but there are also synchronized methods on the CommitTracker scehduleCommitWithin method, run method, and getCommitCount method that will cause similar blocking at least for deletes.

          Show
          Jayson Minard added a comment - - edited This will involve a change to locking as seen in the thread mentioned above between add/delete/commit methods, but there are also synchronized methods on the CommitTracker scehduleCommitWithin method, run method, and getCommitCount method that will cause similar blocking at least for deletes.

            People

            • Assignee:
              Unassigned
              Reporter:
              Jayson Minard
            • Votes:
              5 Vote for this issue
              Watchers:
              11 Start watching this issue

              Dates

              • Created:
                Updated:

                Development