Solr
  1. Solr
  2. SOLR-2280

commitWithin ignored for a delete query

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 3.6, 4.0-ALPHA
    • Component/s: clients - java
    • Labels:
      None

      Description

      The commitWithin option on an UpdateRequest is only honored for requests containing new documents. It does not, for example, work with a delete query. The following doesn't work as expected:

      UpdateRequest request = new UpdateRequest();
      request.deleteById("id123");
      request.setCommitWithin(1000);
      solrServer.request(request);
      

      In my opinion, the commitWithin attribute should be permitted on the <delete/> xml tag as well as <add/>. Such a change would go in XMLLoader.java and its would have some ramifications elsewhere too. Once this is done, then UpdateRequest.getXml() can be updated to generate the right XML.

      1. SOLR-2280.patch
        28 kB
        Jan Høydahl
      2. SOLR-2280.patch
        28 kB
        Jan Høydahl
      3. SOLR-2280.patch
        27 kB
        Jan Høydahl
      4. SOLR-2280.patch
        24 kB
        Jan Høydahl
      5. SOLR-2280.patch
        24 kB
        Juan Grande
      6. SOLR-2280.patch
        24 kB
        Juan Grande
      7. SOLR-2280-3x.patch
        31 kB
        Jan Høydahl
      8. SOLR-2280-3x.patch
        28 kB
        Jan Høydahl
      9. SOLR-2280-3x.patch
        25 kB
        Jan Høydahl

        Activity

        Hide
        Jan Høydahl added a comment -

        Hi huangfox. Please ask questions on the solr-user mailing list, not here. See http://wiki.apache.org/solr/UsingMailingLists

        Show
        Jan Høydahl added a comment - Hi huangfox. Please ask questions on the solr-user mailing list, not here. See http://wiki.apache.org/solr/UsingMailingLists
        Hide
        huangfox added a comment -

        I create a method deleteById(String id , int commitWithinMs) in solrJ(solr3.5),
        but committing is affected by solrconfig.xml(<maxTime>60000</maxTime>)

        Add the document is valid .

        Show
        huangfox added a comment - I create a method deleteById(String id , int commitWithinMs) in solrJ(solr3.5), but committing is affected by solrconfig.xml(<maxTime>60000</maxTime>) – Add the document is valid .
        Hide
        Jan Høydahl added a comment -

        Fixed on trunk and 3x

        Show
        Jan Høydahl added a comment - Fixed on trunk and 3x
        Hide
        Jan Høydahl added a comment -

        Final patches, backported to 3x

        Show
        Jan Høydahl added a comment - Final patches, backported to 3x
        Hide
        Jan Høydahl added a comment -

        New patch against trunk.

        Added support for commitWithin as request parameter on deletes for XML and JSON loaders. Tested ok.

        Think this is getting close. Any other comments? Would like to get this into 3x soon.

        Show
        Jan Høydahl added a comment - New patch against trunk. Added support for commitWithin as request parameter on deletes for XML and JSON loaders. Tested ok. Think this is getting close. Any other comments? Would like to get this into 3x soon.
        Hide
        Juan Grande added a comment -

        Hi Jan, is there something else that I can do to keep this patch moving on?

        Show
        Juan Grande added a comment - Hi Jan, is there something else that I can do to keep this patch moving on?
        Hide
        Jan Høydahl added a comment -

        New patches which adds new commitWithin capable SolrJ methods for deleteBy*()

        Show
        Jan Høydahl added a comment - New patches which adds new commitWithin capable SolrJ methods for deleteBy*()
        Hide
        Jan Høydahl added a comment -

        I also plan to add in support for the convenience methods deleteById(String id, int commitWithinMs) etc in SolrJ the same way as for adds.

        Show
        Jan Høydahl added a comment - I also plan to add in support for the convenience methods deleteById(String id, int commitWithinMs) etc in SolrJ the same way as for adds.
        Hide
        Jan Høydahl added a comment -

        Here's a patch for trunk (SOLR-2280.patch). I've also tested and slightly updated the 3x patch, and named it SOLR-2280-3x.patch.

        PS: Juan - we normally create patches using "project" as root, not solr/.

        Test coverage is good. One or two more eyes on it and I think it's ready for commit.

        Show
        Jan Høydahl added a comment - Here's a patch for trunk ( SOLR-2280 .patch). I've also tested and slightly updated the 3x patch, and named it SOLR-2280 -3x.patch. PS: Juan - we normally create patches using "project" as root, not solr/. Test coverage is good. One or two more eyes on it and I think it's ready for commit.
        Hide
        Juan Grande added a comment -

        Here's the new patch, it applies to 3x branch.

        Show
        Juan Grande added a comment - Here's the new patch, it applies to 3x branch.
        Hide
        Juan Grande added a comment -

        Hi Jan,

        I'm working on it, I will upload a new patch shortly. I'll update the tests to use the same approach as in SOLR-2565.

        Show
        Juan Grande added a comment - Hi Jan, I'm working on it, I will upload a new patch shortly. I'll update the tests to use the same approach as in SOLR-2565 .
        Hide
        Jan Høydahl added a comment -

        Juan, would you care uploading a new patch which applies cleanly to 3.x (or trunk)? This looks like a nice addition to the other commitWithin improvements in 3.x.

        Also, could you find a way to test commitWithin without adding 20 seconds of waitForCommit() to the test time? Waits in the tests have also proven unreliable..

        Show
        Jan Høydahl added a comment - Juan, would you care uploading a new patch which applies cleanly to 3.x (or trunk)? This looks like a nice addition to the other commitWithin improvements in 3.x. Also, could you find a way to test commitWithin without adding 20 seconds of waitForCommit() to the test time? Waits in the tests have also proven unreliable..
        Hide
        Robert Muir added a comment -

        3.4 -> 3.5

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

        I'm submitting a patch that implements commitWithin on deletes. The patch is for the 3x branch.

        Two things should be noted:

        1. The commit is fired even if the delete doesn't really delete any document.
        2. When using the BinaryUpdateRequestHandler the params of the UpdateRequest are loaded when parsing the docs. If the request doesn't include a docs list, then the params aren't loaded. I added a workaround for this, but SOLR-1164 should solve this problem definitely.
        Show
        Juan Grande added a comment - I'm submitting a patch that implements commitWithin on deletes. The patch is for the 3x branch. Two things should be noted: The commit is fired even if the delete doesn't really delete any document. When using the BinaryUpdateRequestHandler the params of the UpdateRequest are loaded when parsing the docs. If the request doesn't include a docs list, then the params aren't loaded. I added a workaround for this, but SOLR-1164 should solve this problem definitely.
        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
        Jan Høydahl added a comment -

        +1 I have client side code explicitly working around this bug, would be nice with a fix in 3.2

        Show
        Jan Høydahl added a comment - +1 I have client side code explicitly working around this bug, would be nice with a fix in 3.2
        Hide
        Yonik Seeley added a comment -

        +1

        Show
        Yonik Seeley added a comment - +1

          People

          • Assignee:
            Juan Grande
            Reporter:
            David Smiley
          • Votes:
            3 Vote for this issue
            Watchers:
            7 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development