Solr
  1. Solr
  2. SOLR-8302

SolrResourceLoader should take a Path to its instance directory, rather than a String

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 5.4, 6.0
    • Component/s: None
    • Labels:
      None

      Description

      First step of SOLR-8282. We have a whole bunch of code that deals with loading things relative to the resource loader's instance dir. These become a lot simpler if the instance dir is a Path.

      1. SOLR-8302.patch
        144 kB
        Alan Woodward
      2. SOLR-8302.patch
        126 kB
        Alan Woodward

        Activity

        Hide
        Alan Woodward added a comment -

        Patch for trunk. This changes the SolrResourceLoader constructors to take Path instead of String, and getInstanceDir() now returns a Path as well. I've tried to keep the effects on everything else as limited as possible, to keep the size of the patch down.

        One question is whether this should be a clean break for 5.4, or if I should add back the altered method signatures as deprecated. SolrResourceLoader is an expert internal API, but there's a possibility that some plugins might use getInstanceDir().

        Show
        Alan Woodward added a comment - Patch for trunk. This changes the SolrResourceLoader constructors to take Path instead of String, and getInstanceDir() now returns a Path as well. I've tried to keep the effects on everything else as limited as possible, to keep the size of the patch down. One question is whether this should be a clean break for 5.4, or if I should add back the altered method signatures as deprecated. SolrResourceLoader is an expert internal API, but there's a possibility that some plugins might use getInstanceDir().
        Hide
        Shawn Heisey added a comment -

        Strong +1 for this idea! I haven't reviewed the patch yet.

        When I started looking at NIO2 conversion for Solr (in general, not limited to this class), I noticed a lot of code that concatenates strings of harcoded filesystem or resource paths with forward slashes. The code would be much cleaner and cross-platform with resolve and other NIO2 methods.

        I personally would be OK with simply changing the API, but the javadoc at the class level does not actually say that it is expert or internal. I'm guessing deprecation will be required, unless it's sufficient to add the internal/expert designation in the javadoc at the same time as this change. I do see one method currently marked as expert, but I don't think that method is affected by this patch.

        Show
        Shawn Heisey added a comment - Strong +1 for this idea! I haven't reviewed the patch yet. When I started looking at NIO2 conversion for Solr (in general, not limited to this class), I noticed a lot of code that concatenates strings of harcoded filesystem or resource paths with forward slashes. The code would be much cleaner and cross-platform with resolve and other NIO2 methods. I personally would be OK with simply changing the API, but the javadoc at the class level does not actually say that it is expert or internal. I'm guessing deprecation will be required, unless it's sufficient to add the internal/expert designation in the javadoc at the same time as this change. I do see one method currently marked as expert, but I don't think that method is affected by this patch.
        Hide
        Alan Woodward added a comment -

        Yes, I think deprecation is probably the way forward. This will mean changing the name of the new method, as Java doesn't let you have identically-named methods with different return types - will work up a patch to change the name to .getInstancePath(), with the deprecated .getInstanceDir() just redirecting to that.

        Show
        Alan Woodward added a comment - Yes, I think deprecation is probably the way forward. This will mean changing the name of the new method, as Java doesn't let you have identically-named methods with different return types - will work up a patch to change the name to .getInstancePath(), with the deprecated .getInstanceDir() just redirecting to that.
        Hide
        Alan Woodward added a comment -

        Updated patch with deprecations.

        Show
        Alan Woodward added a comment - Updated patch with deprecations.
        Hide
        ASF subversion and git services added a comment -

        Commit 1715342 from Alan Woodward in branch 'dev/trunk'
        [ https://svn.apache.org/r1715342 ]

        SOLR-8302: SolrResourceLoader takes a Path for its instance directory

        Show
        ASF subversion and git services added a comment - Commit 1715342 from Alan Woodward in branch 'dev/trunk' [ https://svn.apache.org/r1715342 ] SOLR-8302 : SolrResourceLoader takes a Path for its instance directory
        Hide
        ASF subversion and git services added a comment -

        Commit 1715344 from Alan Woodward in branch 'dev/branches/branch_5x'
        [ https://svn.apache.org/r1715344 ]

        SOLR-8302: SolrResourceLoader takes a Path for its instance directory

        Show
        ASF subversion and git services added a comment - Commit 1715344 from Alan Woodward in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1715344 ] SOLR-8302 : SolrResourceLoader takes a Path for its instance directory
        Hide
        ASF subversion and git services added a comment -

        Commit 1715345 from Alan Woodward in branch 'dev/trunk'
        [ https://svn.apache.org/r1715345 ]

        SOLR-8302: Remove deprecated methods in trunk

        Show
        ASF subversion and git services added a comment - Commit 1715345 from Alan Woodward in branch 'dev/trunk' [ https://svn.apache.org/r1715345 ] SOLR-8302 : Remove deprecated methods in trunk
        Hide
        Alan Woodward added a comment -

        Thanks for the review Shawn

        Show
        Alan Woodward added a comment - Thanks for the review Shawn

          People

          • Assignee:
            Alan Woodward
            Reporter:
            Alan Woodward
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development