Solr
  1. Solr
  2. SOLR-2758

ConcurrentLRUCache should move from common/util to solr/util

    Details

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

      Description

      There is exactly one small dependency that the SolrJ jar has to lucene-core and that is indirectly via ConcurrentLRUCache which is in common/util. SolrJ doesn't even use this cache but it's there any way. Attached is a patch for the move. It also removes the lucene-core dependency that the SolrJ maven pom.xml has on lucene-core.

      Steve Rowe agrees:
      https://issues.apache.org/jira/browse/SOLR-2756?focusedCommentId=13103103&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13103103

        Activity

        David Smiley created issue -
        Hide
        David Smiley added a comment -

        Here is the patch. I use git; hopefully that doesn't matter. We want svn to be aware of the move for history sake.

        Show
        David Smiley added a comment - Here is the patch. I use git; hopefully that doesn't matter. We want svn to be aware of the move for history sake.
        David Smiley made changes -
        Field Original Value New Value
        Attachment SOLR-2758_move_ConcurrentLRUCache.patch [ 12494177 ]
        Hide
        Chris Male added a comment -

        Hey David,

        Since FastLRUCache is in search.*, perhaps ConcurrentLRUCache should be to? for consistency sake.

        I also couldn't get your patch to apply from either commandline or IntelliJ due to inconsistencies in the class level comments in FastLRUCache.

        Show
        Chris Male added a comment - Hey David, Since FastLRUCache is in search.*, perhaps ConcurrentLRUCache should be to? for consistency sake. I also couldn't get your patch to apply from either commandline or IntelliJ due to inconsistencies in the class level comments in FastLRUCache.
        Hide
        David Smiley added a comment -

        I think both caches should go in the same package. These seem like a utility in nature so I think a util package is more appropriate.

        Sorry about the patch formatting. This patch is trivial enough that someone could do it manually.

        Show
        David Smiley added a comment - I think both caches should go in the same package. These seem like a utility in nature so I think a util package is more appropriate. Sorry about the patch formatting. This patch is trivial enough that someone could do it manually.
        Hide
        David Smiley added a comment -

        The attached patch is svn based and it should hopefully work. I did it on trunk.

        I decided against attempting to have FastLRUcache and ConcurrentLRUCache in the same package because FastLRUCache has numerous dependencies on the search package whereas ConcurrentLRUCache has no dependencies on Solr at all; just Lucene's PriorityQueue.

        Show
        David Smiley added a comment - The attached patch is svn based and it should hopefully work. I did it on trunk. I decided against attempting to have FastLRUcache and ConcurrentLRUCache in the same package because FastLRUCache has numerous dependencies on the search package whereas ConcurrentLRUCache has no dependencies on Solr at all; just Lucene's PriorityQueue.
        David Smiley made changes -
        Attachment SOLR-2758_move_ConcurrentLRUCache.patch [ 12494451 ]
        Hide
        David Smiley added a comment -

        Please apply this patch to both 3x and trunk branches. Someone might argue that technically this is a breaking change because a class moved from point A to point B, but it is internal. And config files reference the caches via the "solr." convenience.

        Show
        David Smiley added a comment - Please apply this patch to both 3x and trunk branches. Someone might argue that technically this is a breaking change because a class moved from point A to point B, but it is internal. And config files reference the caches via the "solr." convenience.
        Steve Rowe made changes -
        Assignee Steven Rowe [ steve_rowe ]
        Hide
        Steve Rowe added a comment - - edited

        When I applied the patch (using the 'patch' utility), the file movement didn't happen, so I modified the patch to depend on this svn script having been already run:

        svn mv solr/solrj/src/java/org/apache/solr/common/util/ConcurrentLRUCache.java solr/core/src/java/org/apache/solr/util/
        

        (I generated the patch with svn --no-diff-deleted diff > ..., so that the source file's contents wouldn't be needlessly included in the patch.)

        Also, I added a CHANGES.txt entry.

        I plan on committing this shortly.

        Show
        Steve Rowe added a comment - - edited When I applied the patch (using the 'patch' utility), the file movement didn't happen, so I modified the patch to depend on this svn script having been already run: svn mv solr/solrj/src/java/org/apache/solr/common/util/ConcurrentLRUCache.java solr/core/src/java/org/apache/solr/util/ (I generated the patch with svn --no-diff-deleted diff > ... , so that the source file's contents wouldn't be needlessly included in the patch.) Also, I added a CHANGES.txt entry. I plan on committing this shortly.
        Steve Rowe made changes -
        Attachment SOLR-2758_move_ConcurrentLRUCache.patch [ 12494459 ]
        Hide
        Steve Rowe added a comment -

        Committed to trunk and branch_3x.

        Thanks David!

        Show
        Steve Rowe added a comment - Committed to trunk and branch_3x. Thanks David!
        Steve Rowe made changes -
        Status Open [ 1 ] Resolved [ 5 ]
        Fix Version/s 3.5 [ 12317876 ]
        Fix Version/s 4.0 [ 12314992 ]
        Resolution Fixed [ 1 ]
        Hide
        Uwe Schindler added a comment -

        Bulk close after 3.5 is released

        Show
        Uwe Schindler added a comment - Bulk close after 3.5 is released
        Uwe Schindler made changes -
        Status Resolved [ 5 ] Closed [ 6 ]

          People

          • Assignee:
            Steve Rowe
            Reporter:
            David Smiley
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development