Uploaded image for project: 'Solr'
  1. Solr
  2. SOLR-7729

ConcurrentUpdateSolrClient ignoring the collection parameter in some methods

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 5.1
    • Fix Version/s: 6.1, master (7.0)
    • Component/s: SolrJ
    • Labels:

      Description

      Some of the methods in ConcurrentUpdateSolrClient accept an aditional collection parameter, some of this methods are: add(String collection, SolrInputDocument doc) and request(SolrRequest, String collection).

      This collection parameter is being ignored in this cases but works for others like commit(String collection).

      Shawn Heisey noted that:

      Looking into how an update request actually gets added to the background
      queue in ConcurrentUpdateSolrClient, it appears that the "collection"
      information is ignored before the request is added to the queue.

      From the source, when a commit is issued or the UpdateParams.WAIT_SEARCHER is set in the request params the collection parameter is used, otherwise the request UpdateRequest req is queued without any regarding of the collection.

      1. SOLR-7729.patch
        11 kB
        Mark Miller
      2. SOLR-7729-ConcurrentUpdateSolrClient-collection.patch
        10 kB
        Nicolas Gavalda

        Activity

        Hide
        ngavalda Nicolas Gavalda added a comment -

        Here's a resolution proposal in the attached patch. It fixes the issue on my development environment (solrj 5.2.1).

        I added two unit tests to ConcurrentUpdateSolrClientTest:

        • the simple testCollectionParameters, duplicated from BasicHttpSolrClientTest
        • a second test using SendDocsRunnable to send multiple concurrent add requests, using the collection parameter

        Feel free to modify it (or ask me to rework it) if it doesn't meet solr's quality standards.

        Show
        ngavalda Nicolas Gavalda added a comment - Here's a resolution proposal in the attached patch. It fixes the issue on my development environment (solrj 5.2.1). I added two unit tests to ConcurrentUpdateSolrClientTest: the simple testCollectionParameters, duplicated from BasicHttpSolrClientTest a second test using SendDocsRunnable to send multiple concurrent add requests, using the collection parameter Feel free to modify it (or ask me to rework it) if it doesn't meet solr's quality standards.
        Hide
        jorgelbg Jorge Luis Betancourt Gonzalez added a comment -

        I'm going to give it a try to your patch, I was also working on a fix for this, but I'm out on holidays nevertheless I'm going to give it a try.

        Show
        jorgelbg Jorge Luis Betancourt Gonzalez added a comment - I'm going to give it a try to your patch, I was also working on a fix for this, but I'm out on holidays nevertheless I'm going to give it a try.
        Hide
        ngavalda Nicolas Gavalda added a comment -

        Jorge, did you get a chance to look into the patch?

        Show
        ngavalda Nicolas Gavalda added a comment - Jorge, did you get a chance to look into the patch?
        Hide
        githubbot ASF GitHub Bot added a comment -

        GitHub user nicolasgavalda opened a pull request:

        https://github.com/apache/lucene-solr/pull/24

        SOLR-7729: ConcurrentUpdateSolrClient ignoring the collection parameter in some methods

        This is a fix for SOLR-7729.
        I submitted a similar patch on JIRA for the 5.2.1 version, this is an updated version for the master branch.

        You can merge this pull request into a Git repository by running:

        $ git pull https://github.com/nicolasgavalda/lucene-solr SOLR-7729

        Alternatively you can review and apply these changes as the patch at:

        https://github.com/apache/lucene-solr/pull/24.patch

        To close this pull request, make a commit to your master/trunk branch
        with (at least) the following in the commit message:

        This closes #24


        commit c9c14eb96525b90271ce6acc4594db12d6799bf3
        Author: Nicolas Gavalda <nicolas.gavalda@ametys.org>
        Date: 2016-03-22T16:11:55Z

        SOLR-7729: ConcurrentUpdateSolrClient ignoring the collection parameter
        in some methods.


        Show
        githubbot ASF GitHub Bot added a comment - GitHub user nicolasgavalda opened a pull request: https://github.com/apache/lucene-solr/pull/24 SOLR-7729 : ConcurrentUpdateSolrClient ignoring the collection parameter in some methods This is a fix for SOLR-7729 . I submitted a similar patch on JIRA for the 5.2.1 version, this is an updated version for the master branch. You can merge this pull request into a Git repository by running: $ git pull https://github.com/nicolasgavalda/lucene-solr SOLR-7729 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/lucene-solr/pull/24.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #24 commit c9c14eb96525b90271ce6acc4594db12d6799bf3 Author: Nicolas Gavalda <nicolas.gavalda@ametys.org> Date: 2016-03-22T16:11:55Z SOLR-7729 : ConcurrentUpdateSolrClient ignoring the collection parameter in some methods.
        Hide
        markrmiller@gmail.com Mark Miller added a comment -

        Here is a patch that updates this to trunk.

        This is all kind of awkward because the user can easily already have the collection in the base url the client uses, but what can you do. This at least fixes the bug.

        Show
        markrmiller@gmail.com Mark Miller added a comment - Here is a patch that updates this to trunk. This is all kind of awkward because the user can easily already have the collection in the base url the client uses, but what can you do. This at least fixes the bug.
        Hide
        ngavalda Nicolas Gavalda added a comment -

        IMHO the point of this API is to have a unique SolrClient variable which can be used to query multiple collections. It would be bothersome to have to declare a new SolrClient for each new collection to query.

        I had already updated the patch to trunk in the pull request I submitted two weeks ago, I hope you didn't bother too much with the update

        Once the fix is committed to master, could it be merged in 6.x and maybe in 5.x?

        Show
        ngavalda Nicolas Gavalda added a comment - IMHO the point of this API is to have a unique SolrClient variable which can be used to query multiple collections. It would be bothersome to have to declare a new SolrClient for each new collection to query. I had already updated the patch to trunk in the pull request I submitted two weeks ago, I hope you didn't bother too much with the update Once the fix is committed to master, could it be merged in 6.x and maybe in 5.x?
        Hide
        markrmiller@gmail.com Mark Miller added a comment -

        The problem is that the client is not smart about this at all. If you start it with a collection url and then choose a collection, the behavior is no good. This is a common way to init SolrJ clients, so that API was not very well thought out to begin with IMO.

        Show
        markrmiller@gmail.com Mark Miller added a comment - The problem is that the client is not smart about this at all. If you start it with a collection url and then choose a collection, the behavior is no good. This is a common way to init SolrJ clients, so that API was not very well thought out to begin with IMO.
        Hide
        markrmiller@gmail.com Mark Miller added a comment -

        I had already updated the patch to trunk in the pull request I submitted two weeks ago, I hope you didn't bother too much with the update

        Trunk moves fairly fast sometimes, the pull request patch is what I updated to work with trunk: https://github.com/apache/lucene-solr/pull/24.patch

        Once the fix is committed to master, could it be merged in 6.x and maybe in 5.x?

        It will go in 6.1. Will consider 5.5.1.

        I'll commit this shortly.

        Show
        markrmiller@gmail.com Mark Miller added a comment - I had already updated the patch to trunk in the pull request I submitted two weeks ago, I hope you didn't bother too much with the update Trunk moves fairly fast sometimes, the pull request patch is what I updated to work with trunk: https://github.com/apache/lucene-solr/pull/24.patch Once the fix is committed to master, could it be merged in 6.x and maybe in 5.x? It will go in 6.1. Will consider 5.5.1. I'll commit this shortly.
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 0a5f7f8b5e35a053031cc89b40e7c315cfcef82d in lucene-solr's branch refs/heads/master from markrmiller
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=0a5f7f8 ]

        SOLR-7729: ConcurrentUpdateSolrClient ignores the collection parameter in some methods.

        Show
        jira-bot ASF subversion and git services added a comment - Commit 0a5f7f8b5e35a053031cc89b40e7c315cfcef82d in lucene-solr's branch refs/heads/master from markrmiller [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=0a5f7f8 ] SOLR-7729 : ConcurrentUpdateSolrClient ignores the collection parameter in some methods.
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit bf984af6f0824ea00eddf8732e8d4cf8323da022 in lucene-solr's branch refs/heads/branch_6x from markrmiller
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=bf984af ]

        SOLR-7729: ConcurrentUpdateSolrClient ignores the collection parameter in some methods.

        Show
        jira-bot ASF subversion and git services added a comment - Commit bf984af6f0824ea00eddf8732e8d4cf8323da022 in lucene-solr's branch refs/heads/branch_6x from markrmiller [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=bf984af ] SOLR-7729 : ConcurrentUpdateSolrClient ignores the collection parameter in some methods.
        Hide
        markrmiller@gmail.com Mark Miller added a comment -

        I don't know if I like this for a 5.5.1 release because if you used a collection specific URL and set the collection it would have worked and now it will not. It's not the best even for a 5.6 (may not happen) or 6.1 release, but much more tolerable.

        I'm still wondering what we can try and do about it.

        Show
        markrmiller@gmail.com Mark Miller added a comment - I don't know if I like this for a 5.5.1 release because if you used a collection specific URL and set the collection it would have worked and now it will not. It's not the best even for a 5.6 (may not happen) or 6.1 release, but much more tolerable. I'm still wondering what we can try and do about it.
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 0a5f7f8b5e35a053031cc89b40e7c315cfcef82d in lucene-solr's branch refs/heads/jira/SOLR-8908 from markrmiller
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=0a5f7f8 ]

        SOLR-7729: ConcurrentUpdateSolrClient ignores the collection parameter in some methods.

        Show
        jira-bot ASF subversion and git services added a comment - Commit 0a5f7f8b5e35a053031cc89b40e7c315cfcef82d in lucene-solr's branch refs/heads/jira/ SOLR-8908 from markrmiller [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=0a5f7f8 ] SOLR-7729 : ConcurrentUpdateSolrClient ignores the collection parameter in some methods.
        Hide
        ngavalda Nicolas Gavalda added a comment -

        I upgraded to solr 6, so merging it to the 5.5 branch is not a necessary for me anymore, you can ignore this part if you don't want to change the behavior in 5.5.1.
        Thanks for the commits!

        Show
        ngavalda Nicolas Gavalda added a comment - I upgraded to solr 6, so merging it to the 5.5 branch is not a necessary for me anymore, you can ignore this part if you don't want to change the behavior in 5.5.1. Thanks for the commits!
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user nicolasgavalda closed the pull request at:

        https://github.com/apache/lucene-solr/pull/24

        Show
        githubbot ASF GitHub Bot added a comment - Github user nicolasgavalda closed the pull request at: https://github.com/apache/lucene-solr/pull/24
        Hide
        hossman Hoss Man added a comment -

        Manually correcting fixVersion per Step #S5 of LUCENE-7271

        Show
        hossman Hoss Man added a comment - Manually correcting fixVersion per Step #S5 of LUCENE-7271
        Hide
        steve_rowe Steve Rowe added a comment -

        Mark Miller, should this go into 6.0.1?

        Show
        steve_rowe Steve Rowe added a comment - Mark Miller , should this go into 6.0.1?

          People

          • Assignee:
            markrmiller@gmail.com Mark Miller
            Reporter:
            jorgelbg Jorge Luis Betancourt Gonzalez
          • Votes:
            2 Vote for this issue
            Watchers:
            7 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development