Solr
  1. Solr
  2. SOLR-79

[PATCH] Using system properties in Solr configuration

    Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.2
    • Component/s: None
    • Labels:
      None

      Description

      Actually it is not possible to use system properties for configuring the Solr engine. There should be a way of referencing system properties from solrconfig.xml, like {$prop.name}.

      The attached patch will provide this feature.

      1. solr-config-system-property.patch
        1.0 kB
        Alexander Saar
      2. solr-config-system-property.patch
        3 kB
        Yonik Seeley
      3. solr-config-system-property.patch
        9 kB
        Erik Hatcher
      4. solr-config-system-property.patch
        10 kB
        Erik Hatcher
      5. solr-config-system-property.patch
        10 kB
        Erik Hatcher
      6. solr-config-system-property.patch
        11 kB
        Erik Hatcher

        Activity

        Hide
        Hoss Man added a comment -

        This bug was modified as part of a bulk update using the criteria...

        • Marked ("Resolved" or "Closed") and "Fixed"
        • Had no "Fix Version" versions
        • Was listed in the CHANGES.txt for 1.2

        The Fix Version for all 39 issues found was set to 1.2, email notification
        was suppressed to prevent excessive email.

        For a list of all the issues modified, search jira comments for this
        (hopefully) unique string: 20080415hossman2

        Show
        Hoss Man added a comment - This bug was modified as part of a bulk update using the criteria... Marked ("Resolved" or "Closed") and "Fixed" Had no "Fix Version" versions Was listed in the CHANGES.txt for 1.2 The Fix Version for all 39 issues found was set to 1.2, email notification was suppressed to prevent excessive email. For a list of all the issues modified, search jira comments for this (hopefully) unique string: 20080415hossman2
        Hide
        Yonik Seeley added a comment -

        Very nice, this will make testing easier as well.

        Show
        Yonik Seeley added a comment - Very nice, this will make testing easier as well.
        Hide
        Erik Hatcher added a comment -

        This feature has been added, complete with the requested exception handling when a property is not defined and no default value provided.

        Show
        Erik Hatcher added a comment - This feature has been added, complete with the requested exception handling when a property is not defined and no default value provided.
        Hide
        Hoss Man added a comment -

        my view is:

        • in most cases, a system property not being defined is going to prevent the value from being usable (ie: a blank dataDIr)
        • if you want to use a systemp prop in a place where it's acceptible to not have the prop defined at all, then just use $ {prop:}

          to denote that an empty string should be used as the default.

        Show
        Hoss Man added a comment - my view is: in most cases, a system property not being defined is going to prevent the value from being usable (ie: a blank dataDIr) if you want to use a systemp prop in a place where it's acceptible to not have the prop defined at all, then just use $ {prop:} to denote that an empty string should be used as the default.
        Hide
        Erik Hatcher added a comment -

        I dunno.... I suppose throwing an exception is fine though that might
        prevent an otherwise functional Solr instance to start up. I can see
        it being ok either way on this issue, but I can't envision a case
        where I'd define $

        {...}

        bits in my config files and not setting the
        appropriate system properties.

        I'll modify to throw an exception. And commit!

        Erik

        Show
        Erik Hatcher added a comment - I dunno.... I suppose throwing an exception is fine though that might prevent an otherwise functional Solr instance to start up. I can see it being ok either way on this issue, but I can't envision a case where I'd define $ {...} bits in my config files and not setting the appropriate system properties. I'll modify to throw an exception. And commit! Erik
        Hide
        Hoss Man added a comment -

        I was going to chime in that unset properties should probably not default to an empty string:
        1) so that SOLR-109 would be more feasible
        2) so that we don't silently fail on typos

        ...looking at the latest patch, #1 isn't really an issue since $$

        {literal}

        works ... but especially now that it's possible to specify a default value to use if they property isn't set: trying to use an unset property without a default specified should probably result in an exception ... correct?

        as for my comment about system properties for testing: i was smoking crack thinking that the <syspropertyset> already in the build.xml junit target would magically take care of this for us, forgetting that somewhere the properties actually need to be set.

        Show
        Hoss Man added a comment - I was going to chime in that unset properties should probably not default to an empty string: 1) so that SOLR-109 would be more feasible 2) so that we don't silently fail on typos ...looking at the latest patch, #1 isn't really an issue since $$ {literal} works ... but especially now that it's possible to specify a default value to use if they property isn't set: trying to use an unset property without a default specified should probably result in an exception ... correct? as for my comment about system properties for testing: i was smoking crack thinking that the <syspropertyset> already in the build.xml junit target would magically take care of this for us, forgetting that somewhere the properties actually need to be set.
        Hide
        Erik Hatcher added a comment -

        Updated the patch per Hoss' feedback. Specifically:

        i haven't tested Erik's patch, but it reads clean to me.

        a couple of minor nits...

        1) Javadoc: done.

        2) Hoss: if the system properties used were "solr.test.sys.prop1", etc. the patch wouldn't need to modify build.xml ... if we are going to set them explicitly in build.xml, there shold be a comment explaining which test uses them. [Erik: I don't understand... are you saying there is a predefined solr.test.sys.prop1 somewhere? I didn't spot anything like that, so I've kept with defining a couple of test ones in build.xml

        3) ye of little faith! the doc that the Config object holds has been modified recursively, so all methods querying the doc will see the substituted values. I've added a test to show the case you mentioned.

        4) I added similar tests for .evaluate and .getNode. All is well.

        Show
        Erik Hatcher added a comment - Updated the patch per Hoss' feedback. Specifically: i haven't tested Erik's patch, but it reads clean to me. a couple of minor nits... 1) Javadoc: done. 2) Hoss: if the system properties used were "solr.test.sys.prop1", etc. the patch wouldn't need to modify build.xml ... if we are going to set them explicitly in build.xml, there shold be a comment explaining which test uses them. [Erik: I don't understand... are you saying there is a predefined solr.test.sys.prop1 somewhere? I didn't spot anything like that, so I've kept with defining a couple of test ones in build.xml 3) ye of little faith! the doc that the Config object holds has been modified recursively, so all methods querying the doc will see the substituted values. I've added a test to show the case you mentioned. 4) I added similar tests for .evaluate and .getNode. All is well.
        Hide
        Erik Hatcher added a comment -

        Here's the patch with default value implemented. $

        {system.property:default value}

        - default value is optional, if omitted, "" will be used.

        Show
        Erik Hatcher added a comment - Here's the patch with default value implemented. $ {system.property:default value} - default value is optional, if omitted, "" will be used.
        Hide
        Hoss Man added a comment -

        i haven't tested Erik's patch, but it reads clean to me.

        a couple of minor nits...

        1) it would be good to have some javadocs on substituteSystemProperties(Node)

        2) if the system properties used were "solr.test.sys.prop1", etc. the patch wouldn't need to modify build.xml ... if we are going to set them explicitly in build.xml, there shold be a comment explaining which test uses them.

        3) at first glance, i'm not sure if this would work, it would be nice to have a test for it...

        s = SolrConfig.config.get("propTest/[@attr='prop two']", "default");
        assertEquals("prefix-prop one-prop two-suffix", s);

        4) it looks like it should work, but tests of Config.evaluate, Config.getNode, and the various DOMUtil methods would also be good

        Show
        Hoss Man added a comment - i haven't tested Erik's patch, but it reads clean to me. a couple of minor nits... 1) it would be good to have some javadocs on substituteSystemProperties(Node) 2) if the system properties used were "solr.test.sys.prop1", etc. the patch wouldn't need to modify build.xml ... if we are going to set them explicitly in build.xml, there shold be a comment explaining which test uses them. 3) at first glance, i'm not sure if this would work, it would be nice to have a test for it... s = SolrConfig.config.get("propTest/ [@attr='prop two'] ", "default"); assertEquals("prefix-prop one-prop two-suffix", s); 4) it looks like it should work, but tests of Config.evaluate, Config.getNode, and the various DOMUtil methods would also be good
        Hide
        Erik Hatcher added a comment -

        Update patch to include necessary build.xml changes and additional test for subtitution in Ant property.

        Show
        Erik Hatcher added a comment - Update patch to include necessary build.xml changes and additional test for subtitution in Ant property.
        Hide
        Erik Hatcher added a comment -

        Borrowing some code from Ant for property substitution (ugly, but avoids reinventing the wheel), attached is a patch that implements property substituion on all attributes and text nodes in the loaded DOM. The substituions are done in-place on the DOM thus avoiding having to change code in multiple places.

        Show
        Erik Hatcher added a comment - Borrowing some code from Ant for property substitution (ugly, but avoids reinventing the wheel), attached is a patch that implements property substituion on all attributes and text nodes in the loaded DOM. The substituions are done in-place on the DOM thus avoiding having to change code in multiple places.
        Hide
        Alexander Saar added a comment -

        I took the {$system.prop} pattern from Avalon configuration style. After having a look at the ant way, I would also prefer $

        {system.prop}.

        Furthermore I think it will be very helpful to have something like ${system.prop}

        /bin. Actually it was not my use case, but I'm sure that it is helpful for somebody.

        Show
        Alexander Saar added a comment - I took the {$system.prop} pattern from Avalon configuration style. After having a look at the ant way, I would also prefer $ {system.prop}. Furthermore I think it will be very helpful to have something like ${system.prop} /bin. Actually it was not my use case, but I'm sure that it is helpful for somebody.
        Hide
        Hoss Man added a comment -

        1) i'm not really a fan of the Jetty syntax ... much too verbose for my taste, I would prefer $

        {system.prop}

        over {$system.prop} to be consistent with ant and shell variables ... but it would be nice to still support defaults ... isn't $

        {prop.name:default val}

        what bash uses?

        2) to be useful, wouldn't we want this to work even if the property wasn't the full value of the tag, ie...

        <listener event="postCommit" class="solr.RunExecutableListener">
        <str name="exe">snapshooter</str>
        <str name="dir">$

        {some.dir.property}

        /bin</str>
        <bool name="wait">true</bool>
        </listener>

        ...if we do this, we should also have some sort of escaping mechanism for completleness, is <tag>$$

        {some.prop}

        </tag> would not get substitution.

        3) not everything uses Config.getVal ... we would also need similar substitution logic in Config.evaluate, Config.getNode, and the various DOMUtil methods so that it could be used anywhere in the solrconfig.xml ... walking all of the NodeTrees to make the substitutions at every level, might get complicated ... maybe there is an easier way of adding a "visiter" to the DOM model?

        Show
        Hoss Man added a comment - 1) i'm not really a fan of the Jetty syntax ... much too verbose for my taste, I would prefer $ {system.prop} over {$system.prop} to be consistent with ant and shell variables ... but it would be nice to still support defaults ... isn't $ {prop.name:default val} what bash uses? 2) to be useful, wouldn't we want this to work even if the property wasn't the full value of the tag, ie... <listener event="postCommit" class="solr.RunExecutableListener"> <str name="exe">snapshooter</str> <str name="dir">$ {some.dir.property} /bin</str> <bool name="wait">true</bool> </listener> ...if we do this, we should also have some sort of escaping mechanism for completleness, is <tag>$$ {some.prop} </tag> would not get substitution. 3) not everything uses Config.getVal ... we would also need similar substitution logic in Config.evaluate, Config.getNode, and the various DOMUtil methods so that it could be used anywhere in the solrconfig.xml ... walking all of the NodeTrees to make the substitutions at every level, might get complicated ... maybe there is an easier way of adding a "visiter" to the DOM model?
        Hide
        Mike Klaas added a comment -

        FWIW, Jetty uses the following xml:

        <SystemProperty name="jetty.home" default="."/>/logs/yyyy_mm_dd.jet
        ty.log

        Show
        Mike Klaas added a comment - FWIW, Jetty uses the following xml: <SystemProperty name="jetty.home" default="."/>/logs/yyyy_mm_dd.jet ty.log
        Hide
        Yonik Seeley added a comment -

        updated patch with test.

        Is there a particular reason that the pattern {$system.prop} was picked over $

        {system.prop}

        ?

        Show
        Yonik Seeley added a comment - updated patch with test. Is there a particular reason that the pattern {$system.prop} was picked over $ {system.prop} ?
        Hide
        Yonik Seeley added a comment -

        Interesting idea, thanks Alexander!
        If no one has objections, I think it should go in.

        Show
        Yonik Seeley added a comment - Interesting idea, thanks Alexander! If no one has objections, I think it should go in.
        Hide
        Alexander Saar added a comment -

        Patch that allows usage of references to system properties in the solrconfig.xml

        Show
        Alexander Saar added a comment - Patch that allows usage of references to system properties in the solrconfig.xml

          People

          • Assignee:
            Erik Hatcher
            Reporter:
            Alexander Saar
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development