Solr
  1. Solr
  2. SOLR-670

UpdateHandler must provide a rollback feature

    Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.3
    • Fix Version/s: 1.4
    • Component/s: search
    • Labels:
      None

      Description

      Lucene IndexWriter already has a rollback method. There should be a counterpart for the same in UpdateHandler so that users can do a rollback over http

      1. SOLR-670.patch
        3 kB
        Shalin Shekhar Mangar
      2. SOLR-670.patch
        3 kB
        Shalin Shekhar Mangar
      3. SOLR-670.patch
        23 kB
        Shalin Shekhar Mangar
      4. SOLR-670.patch
        23 kB
        Koji Sekiguchi
      5. SOLR-670.patch
        17 kB
        Koji Sekiguchi
      6. SOLR-670-revert-cumulative-counts.patch
        4 kB
        Koji Sekiguchi

        Activity

        Hide
        Koji Sekiguchi added a comment -

        A patch supports the rollback feature.

        One question comes up. Is it worthy to have postRollback() method in SolrEventListener? Because SolrEventListener is an interface, if we add the method, it will break back-compat.

        Show
        Koji Sekiguchi added a comment - A patch supports the rollback feature. One question comes up. Is it worthy to have postRollback() method in SolrEventListener? Because SolrEventListener is an interface, if we add the method, it will break back-compat.
        Hide
        Koji Sekiguchi added a comment -

        Updated patch which includes commit and rollback test. The test looks like:

        pseudo code
          public void testUncommit() throws Exception {
            add(doc("A"));
            search("A");  // "A" should not be found.
          }
        
          public void testAddCommit() throws Exception {
            add(doc("A"));
            commit();
            search("A");  // "A" should be found.
          }
        
          public void testDeleteCommit() throws Exception {
            add(doc("A"));
            add(doc("B"));
            commit();
            search("A OR B");  // "A" and "B" should be found.
            delete(doc("B"));
            search("A OR B");  // "A" and "B" should be found.
            commit();
            search("A OR B");  // "B" should not be found.
          }
        
          public void testAddRollback() throws Exception {
            add(doc("A"));
            commit();
            add(doc("B"));
            rollback();
            commit();
            search("A OR B");  // "B" should not be found.
          }
        
          public void testDeleteRollback() throws Exception {
            add(doc("A"));
            add(doc("B"));
            commit();
            search("A OR B");  // "A" and "B" should be found.
            delete(doc("B"));
            rollback();
            commit();
            search("A OR B");  // "A" and "B" should be found.
          }
        
        Show
        Koji Sekiguchi added a comment - Updated patch which includes commit and rollback test. The test looks like: pseudo code public void testUncommit() throws Exception { add(doc( "A" )); search( "A" ); // "A" should not be found. } public void testAddCommit() throws Exception { add(doc( "A" )); commit(); search( "A" ); // "A" should be found. } public void testDeleteCommit() throws Exception { add(doc( "A" )); add(doc( "B" )); commit(); search( "A OR B" ); // "A" and "B" should be found. delete(doc( "B" )); search( "A OR B" ); // "A" and "B" should be found. commit(); search( "A OR B" ); // "B" should not be found. } public void testAddRollback() throws Exception { add(doc( "A" )); commit(); add(doc( "B" )); rollback(); commit(); search( "A OR B" ); // "B" should not be found. } public void testDeleteRollback() throws Exception { add(doc( "A" )); add(doc( "B" )); commit(); search( "A OR B" ); // "A" and "B" should be found. delete(doc( "B" )); rollback(); commit(); search( "A OR B" ); // "A" and "B" should be found. }
        Hide
        Shalin Shekhar Mangar added a comment -

        Thanks for the patch Koji. Updated with a few changes:

        1. CSVRequestHandler#handleRequestBody should call handleRollback if streams is null just like it calls handleCommit
        2. XmlUpdateRequestHandler#handleRequestBody should call handleRollback if streams is null just like it calls handleCommit

        Let us think about SolrEventListener changes if users ask for it. Off hand, I can't think of a use-case.

        I shall commit this shortly.

        Show
        Shalin Shekhar Mangar added a comment - Thanks for the patch Koji. Updated with a few changes: CSVRequestHandler#handleRequestBody should call handleRollback if streams is null just like it calls handleCommit XmlUpdateRequestHandler#handleRequestBody should call handleRollback if streams is null just like it calls handleCommit Let us think about SolrEventListener changes if users ask for it. Off hand, I can't think of a use-case. I shall commit this shortly.
        Hide
        Shalin Shekhar Mangar added a comment -

        Forgot to upload the patch

        Show
        Shalin Shekhar Mangar added a comment - Forgot to upload the patch
        Hide
        Shalin Shekhar Mangar added a comment -

        Committed revision 704853.

        Thanks Noble and Koji!

        Show
        Shalin Shekhar Mangar added a comment - Committed revision 704853. Thanks Noble and Koji!
        Hide
        Otis Gospodnetic added a comment -

        Is it possible that the new rollback causes the IndexWriter to be closed on error, which then causes the following error next time you try to add a (valid) document?

        Feb 10, 2009 5:46:28 PM org.apache.solr.update.processor.LogUpdateProcessor finish
        INFO: {} 0 1
        Feb 10, 2009 5:46:28 PM org.apache.solr.common.SolrException log
        SEVERE: org.apache.lucene.store.AlreadyClosedException: this IndexWriter is closed
        at org.apache.lucene.index.IndexWriter.ensureOpen(IndexWriter.java:397)
        at org.apache.lucene.index.IndexWriter.ensureOpen(IndexWriter.java:402)
        at org.apache.lucene.index.IndexWriter.updateDocument(IndexWriter.java:2108)
        at org.apache.solr.update.DirectUpdateHandler2.addDoc(DirectUpdateHandler2.java:218)
        at org.apache.solr.update.processor.RunUpdateProcessor.processAdd(RunUpdateProcessorFactory.java:60)
        at org.apache.solr.handler.XMLLoader.processUpdate(XMLLoader.java:140)
        at org.apache.solr.handler.XMLLoader.load(XMLLoader.java:69)
        at org.apache.solr.handler.ContentStreamHandlerBase.handleRequestBody(ContentStreamHandlerBase.java:54)
        at org.apache.solr.handler.RequestHandlerBase.handleRequest(RequestHandlerBase.java:131)
        at org.apache.solr.core.SolrCore.execute(SolrCore.java:1313)
        at org.apache.solr.servlet.SolrDispatchFilter.execute(SolrDispatchFilter.java:303)
        at org.apache.solr.servlet.SolrDispatchFilter.doFilter(SolrDispatchFilter.java:232)
        at org.mortbay.jetty.servlet.ServletHandler$CachedChain.doFilter(ServletHandler.java:1089)
        at org.mortbay.jetty.servlet.ServletHandler.handle(ServletHandler.java:365)
        at org.mortbay.jetty.security.SecurityHandler.handle(SecurityHandler.java:216)
        at org.mortbay.jetty.servlet.SessionHandler.handle(SessionHandler.java:181)
        at org.mortbay.jetty.handler.ContextHandler.handle(ContextHandler.java:712)
        at org.mortbay.jetty.webapp.WebAppContext.handle(WebAppContext.java:405)
        at org.mortbay.jetty.handler.ContextHandlerCollection.handle(ContextHandlerCollection.java:211)
        at org.mortbay.jetty.handler.HandlerCollection.handle(HandlerCollection.java:114)
        at org.mortbay.jetty.handler.HandlerWrapper.handle(HandlerWrapper.java:139)
        at org.mortbay.jetty.Server.handle(Server.java:285)
        at org.mortbay.jetty.HttpConnection.handleRequest(HttpConnection.java:502)
        at org.mortbay.jetty.HttpConnection$RequestHandler.content(HttpConnection.java:835)
        at org.mortbay.jetty.HttpParser.parseNext(HttpParser.java:641)
        at org.mortbay.jetty.HttpParser.parseAvailable(HttpParser.java:202)
        at org.mortbay.jetty.HttpConnection.handle(HttpConnection.java:378)
        at org.mortbay.jetty.bio.SocketConnector$Connection.run(SocketConnector.java:226)
        at org.mortbay.thread.BoundedThreadPool$PoolThread.run(BoundedThreadPool.java:442)

        After rollback is invoked, is one supposed to execute some other command to get Solr in a healthy state?

        Show
        Otis Gospodnetic added a comment - Is it possible that the new rollback causes the IndexWriter to be closed on error, which then causes the following error next time you try to add a (valid) document? Feb 10, 2009 5:46:28 PM org.apache.solr.update.processor.LogUpdateProcessor finish INFO: {} 0 1 Feb 10, 2009 5:46:28 PM org.apache.solr.common.SolrException log SEVERE: org.apache.lucene.store.AlreadyClosedException: this IndexWriter is closed at org.apache.lucene.index.IndexWriter.ensureOpen(IndexWriter.java:397) at org.apache.lucene.index.IndexWriter.ensureOpen(IndexWriter.java:402) at org.apache.lucene.index.IndexWriter.updateDocument(IndexWriter.java:2108) at org.apache.solr.update.DirectUpdateHandler2.addDoc(DirectUpdateHandler2.java:218) at org.apache.solr.update.processor.RunUpdateProcessor.processAdd(RunUpdateProcessorFactory.java:60) at org.apache.solr.handler.XMLLoader.processUpdate(XMLLoader.java:140) at org.apache.solr.handler.XMLLoader.load(XMLLoader.java:69) at org.apache.solr.handler.ContentStreamHandlerBase.handleRequestBody(ContentStreamHandlerBase.java:54) at org.apache.solr.handler.RequestHandlerBase.handleRequest(RequestHandlerBase.java:131) at org.apache.solr.core.SolrCore.execute(SolrCore.java:1313) at org.apache.solr.servlet.SolrDispatchFilter.execute(SolrDispatchFilter.java:303) at org.apache.solr.servlet.SolrDispatchFilter.doFilter(SolrDispatchFilter.java:232) at org.mortbay.jetty.servlet.ServletHandler$CachedChain.doFilter(ServletHandler.java:1089) at org.mortbay.jetty.servlet.ServletHandler.handle(ServletHandler.java:365) at org.mortbay.jetty.security.SecurityHandler.handle(SecurityHandler.java:216) at org.mortbay.jetty.servlet.SessionHandler.handle(SessionHandler.java:181) at org.mortbay.jetty.handler.ContextHandler.handle(ContextHandler.java:712) at org.mortbay.jetty.webapp.WebAppContext.handle(WebAppContext.java:405) at org.mortbay.jetty.handler.ContextHandlerCollection.handle(ContextHandlerCollection.java:211) at org.mortbay.jetty.handler.HandlerCollection.handle(HandlerCollection.java:114) at org.mortbay.jetty.handler.HandlerWrapper.handle(HandlerWrapper.java:139) at org.mortbay.jetty.Server.handle(Server.java:285) at org.mortbay.jetty.HttpConnection.handleRequest(HttpConnection.java:502) at org.mortbay.jetty.HttpConnection$RequestHandler.content(HttpConnection.java:835) at org.mortbay.jetty.HttpParser.parseNext(HttpParser.java:641) at org.mortbay.jetty.HttpParser.parseAvailable(HttpParser.java:202) at org.mortbay.jetty.HttpConnection.handle(HttpConnection.java:378) at org.mortbay.jetty.bio.SocketConnector$Connection.run(SocketConnector.java:226) at org.mortbay.thread.BoundedThreadPool$PoolThread.run(BoundedThreadPool.java:442) After rollback is invoked, is one supposed to execute some other command to get Solr in a healthy state?
        Hide
        Shalin Shekhar Mangar added a comment -

        Otis, which Solr version are you using or more specifically what is the revision of Lucene jars? Did you call commit after the rollback?

        Is it possible that the new rollback causes the IndexWriter to be closed on error, which then causes the following error next time you try to add a (valid) document?

        The javadocs for IndexWriter#rollback do not say anything like that, I'll try to reproduce the problem with trunk.

        Show
        Shalin Shekhar Mangar added a comment - Otis, which Solr version are you using or more specifically what is the revision of Lucene jars? Did you call commit after the rollback? Is it possible that the new rollback causes the IndexWriter to be closed on error, which then causes the following error next time you try to add a (valid) document? The javadocs for IndexWriter#rollback do not say anything like that, I'll try to reproduce the problem with trunk.
        Hide
        Otis Gospodnetic added a comment -

        That was with Solr trunk (svn up-ed right before trying).
        I did not call commit after rollback when that happened, though I think I tried adding commit, too, and that didn't do anything either.

        Show
        Otis Gospodnetic added a comment - That was with Solr trunk (svn up-ed right before trying). I did not call commit after rollback when that happened, though I think I tried adding commit, too, and that didn't do anything either.
        Hide
        Shalin Shekhar Mangar added a comment -

        I commented out the commit in DirectUpdateHandlerTest#testAddRollback and I saw the same exception.

        org.apache.lucene.store.AlreadyClosedException: this IndexWriter is closed
        at org.apache.lucene.index.IndexWriter.ensureOpen(IndexWriter.java:410)
        at org.apache.lucene.index.IndexWriter.ensureOpen(IndexWriter.java:415)
        at org.apache.lucene.index.IndexWriter.updateDocument(IndexWriter.java:2170)
        at org.apache.solr.update.DirectUpdateHandler2.addDoc(DirectUpdateHandler2.java:234)
        at org.apache.solr.update.DirectUpdateHandlerTest.addSimpleDoc(DirectUpdateHandlerTest.java:254)
        at org.apache.solr.update.DirectUpdateHandlerTest.testAddRollback(DirectUpdateHandlerTest.java:188)

        Now that I'm looking at this again, I don't see why a commit should be necessary at all. If DUH2#rollbackWriter sets writer=null, then we wouldn't need to call commit at all and we don't really need to refresh the IndexSearcher because the index does not change. I'll re-open the issue and attach a patch.

        Show
        Shalin Shekhar Mangar added a comment - I commented out the commit in DirectUpdateHandlerTest#testAddRollback and I saw the same exception. org.apache.lucene.store.AlreadyClosedException: this IndexWriter is closed at org.apache.lucene.index.IndexWriter.ensureOpen(IndexWriter.java:410) at org.apache.lucene.index.IndexWriter.ensureOpen(IndexWriter.java:415) at org.apache.lucene.index.IndexWriter.updateDocument(IndexWriter.java:2170) at org.apache.solr.update.DirectUpdateHandler2.addDoc(DirectUpdateHandler2.java:234) at org.apache.solr.update.DirectUpdateHandlerTest.addSimpleDoc(DirectUpdateHandlerTest.java:254) at org.apache.solr.update.DirectUpdateHandlerTest.testAddRollback(DirectUpdateHandlerTest.java:188) Now that I'm looking at this again, I don't see why a commit should be necessary at all. If DUH2#rollbackWriter sets writer=null, then we wouldn't need to call commit at all and we don't really need to refresh the IndexSearcher because the index does not change. I'll re-open the issue and attach a patch.
        Hide
        Shalin Shekhar Mangar added a comment -

        Re-opening per above comment

        Show
        Shalin Shekhar Mangar added a comment - Re-opening per above comment
        Hide
        Shalin Shekhar Mangar added a comment -
        1. Set writer=null in a finally block in DUH2#rollbackWriter so that there is no need to call commit after a rollback
        2. Update DirectUpdateHandlerTest to not call commit and to make sure we can still add documents.
        Show
        Shalin Shekhar Mangar added a comment - Set writer=null in a finally block in DUH2#rollbackWriter so that there is no need to call commit after a rollback Update DirectUpdateHandlerTest to not call commit and to make sure we can still add documents.
        Hide
        Koji Sekiguchi added a comment -

        Thank you guys. I didn't recall why I didn't set writer=null in the finally block when I wrote the first patch... I'd like to try Shalin's patch, but I couldn't apply it:

        [koji@macbook SOLR-670]$ patch -p0 --dry-run < SOLR-670.patch 
        patching file src/test/org/apache/solr/update/DirectUpdateHandlerTest.java
        Hunk #1 FAILED at 173.
        Hunk #2 FAILED at 183.
        Hunk #3 FAILED at 229.
        Hunk #4 FAILED at 236.
        4 out of 4 hunks FAILED -- saving rejects to file src/test/org/apache/solr/update/DirectUpdateHandlerTest.java.rej
        patching file src/java/org/apache/solr/update/DirectUpdateHandler2.java
        
        Show
        Koji Sekiguchi added a comment - Thank you guys. I didn't recall why I didn't set writer=null in the finally block when I wrote the first patch... I'd like to try Shalin's patch, but I couldn't apply it: [koji@macbook SOLR-670]$ patch -p0 --dry-run < SOLR-670.patch patching file src/test/org/apache/solr/update/DirectUpdateHandlerTest.java Hunk #1 FAILED at 173. Hunk #2 FAILED at 183. Hunk #3 FAILED at 229. Hunk #4 FAILED at 236. 4 out of 4 hunks FAILED -- saving rejects to file src/test/org/apache/solr/update/DirectUpdateHandlerTest.java.rej patching file src/java/org/apache/solr/update/DirectUpdateHandler2.java
        Hide
        Shalin Shekhar Mangar added a comment -

        There was some problem with the previous patch.

        Koji, can you please try this one?

        Show
        Shalin Shekhar Mangar added a comment - There was some problem with the previous patch. Koji, can you please try this one?
        Hide
        Koji Sekiguchi added a comment -

        Shalin, the patch looks fine. +1 to commit.

        Show
        Koji Sekiguchi added a comment - Shalin, the patch looks fine. +1 to commit.
        Hide
        Shalin Shekhar Mangar added a comment -

        Committed revision 743359.

        I'll update the wiki with more details on the rollback command.

        Show
        Shalin Shekhar Mangar added a comment - Committed revision 743359. I'll update the wiki with more details on the rollback command.
        Hide
        Koji Sekiguchi added a comment -

        Rollback should reset not only adds/deletesById/deletesByQuery counts but also cumulative counts of them.

        Show
        Koji Sekiguchi added a comment - Rollback should reset not only adds/deletesById/deletesByQuery counts but also cumulative counts of them.
        Hide
        Koji Sekiguchi added a comment -

        The fix and test case. I'll commit soon.

        Show
        Koji Sekiguchi added a comment - The fix and test case. I'll commit soon.
        Hide
        Koji Sekiguchi added a comment -

        Committed revision 824380.

        Show
        Koji Sekiguchi added a comment - Committed revision 824380.
        Hide
        Grant Ingersoll added a comment -

        Bulk close for Solr 1.4

        Show
        Grant Ingersoll added a comment - Bulk close for Solr 1.4

          People

          • Assignee:
            Koji Sekiguchi
            Reporter:
            Noble Paul
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development