Solr
  1. Solr
  2. SOLR-7179

JettySolrRunner should not use system properties to pass configuration

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None

      Description

      JettySolrRunner sets a bunch of system properties to pass configuration down to its SolrDispatchFilter and CoreContainer. This means that we can't launch multiple runners at the same time - sys props must be set up, the runner launched, we wait for solr to start up, then sys props must be changed before the next runner can be instantiated.

      Instead we should be able to pass the relevant properties down to SolrDispatchFilter via the servlet context. This will allow MiniSolrCloudCluster and the various ZK test cases to start and stop their jetties in parallel, which will speed up individual tests (and with luck speed up the test suite overall as well).

      1. SOLR-7179.patch
        72 kB
        Alan Woodward
      2. SOLR-7179.patch
        56 kB
        Alan Woodward
      3. SOLR-7179.patch
        42 kB
        Alan Woodward

        Activity

        Hide
        Alan Woodward added a comment -

        Patch.

        SolrDispatchFilter checks its servlet context for solr home and node properties on init, defaulting to the usual ways of getting these from sys props if the attributes aren't set. JettySolrRunner takes a Properties object as a construction parameter, and the various setXXX methods are deprecated. These were trappy anyway, as they only worked if you had a solr.xml or core.properties set to use specific sysprops.

        Show
        Alan Woodward added a comment - Patch. SolrDispatchFilter checks its servlet context for solr home and node properties on init, defaulting to the usual ways of getting these from sys props if the attributes aren't set. JettySolrRunner takes a Properties object as a construction parameter, and the various setXXX methods are deprecated. These were trappy anyway, as they only worked if you had a solr.xml or core.properties set to use specific sysprops.
        Hide
        Alan Woodward added a comment -

        This patch fixes some test failures due to some odd jetty race conditions

        Show
        Alan Woodward added a comment - This patch fixes some test failures due to some odd jetty race conditions
        Hide
        Mark Miller added a comment - - edited

        Yeah, being able to launch and stop jetties in parallel will be a big savings. I started down this path once, but ran into some crap and moved onto other things.

        Show
        Mark Miller added a comment - - edited Yeah, being able to launch and stop jetties in parallel will be a big savings. I started down this path once, but ran into some crap and moved onto other things.
        Hide
        Alan Woodward added a comment -

        Still chasing down some errors due to various tests setting coreRootDirectory in system properties. Will hopefully be able to commit this in the next couple of days though.

        Show
        Alan Woodward added a comment - Still chasing down some errors due to various tests setting coreRootDirectory in system properties. Will hopefully be able to commit this in the next couple of days though.
        Hide
        Alan Woodward added a comment -

        Final patch. Some funkiness introduced due to SOLR-7171, and we should look at reducing the multiplicity of startJetty() commands in the tests (or better, to start migrating tests to use MiniSolrCloudCluster). But that's for later.

        Show
        Alan Woodward added a comment - Final patch. Some funkiness introduced due to SOLR-7171 , and we should look at reducing the multiplicity of startJetty() commands in the tests (or better, to start migrating tests to use MiniSolrCloudCluster). But that's for later.
        Hide
        ASF subversion and git services added a comment -

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

        SOLR-7179: Stop using sysprops to configure test jettys

        Show
        ASF subversion and git services added a comment - Commit 1664292 from Alan Woodward in branch 'dev/trunk' [ https://svn.apache.org/r1664292 ] SOLR-7179 : Stop using sysprops to configure test jettys
        Hide
        ASF subversion and git services added a comment -

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

        SOLR-7179: Stop using sysprops to configure test jettys

        Show
        ASF subversion and git services added a comment - Commit 1664296 from Alan Woodward in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1664296 ] SOLR-7179 : Stop using sysprops to configure test jettys
        Hide
        Noble Paul added a comment -

        I came across a problem of not being able to use System.getProperty() in a static context of a class. If it was the only test , it would pass , but if I run tests in a batch (means the static is initialized already) it fails

        Show
        Noble Paul added a comment - I came across a problem of not being able to use System.getProperty() in a static context of a class. If it was the only test , it would pass , but if I run tests in a batch (means the static is initialized already) it fails
        Hide
        Alan Woodward added a comment -

        Not sure I follow, Noble. This change means that you shouldn't use system properties for solr config.

        Show
        Alan Woodward added a comment - Not sure I follow, Noble. This change means that you shouldn't use system properties for solr config.
        Hide
        Noble Paul added a comment -

        I have a property called enable.runtime.lib
        I used initialized it as follows

        public static final boolean enableRuntimeLib = System.getProperty("enable.runtime.lib","false")

        And I do a Sytem.setProperty("enable.runtime.lib","true") in the @BeforeClass method of my test. The test would pass if that is the only test

        If I run the entire set of tests . This fails because the static variable is already initialized. I guess the classes are pre-initialized by the time the testcase is reached

        Show
        Noble Paul added a comment - I have a property called enable.runtime.lib I used initialized it as follows public static final boolean enableRuntimeLib = System.getProperty("enable.runtime.lib","false") And I do a Sytem.setProperty("enable.runtime.lib","true") in the @BeforeClass method of my test. The test would pass if that is the only test If I run the entire set of tests . This fails because the static variable is already initialized. I guess the classes are pre-initialized by the time the testcase is reached

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development