Solr
  1. Solr
  2. SOLR-6943

HdfsDirectoryFactory should fall back to system props for most of it's config if it is not found in solrconfig.xml.

    Details

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

      Description

      The new server and config sets has undone the work I did to make hdfs easy out of the box. Rather than count on config for that, we should just allow most of this config to be specified at the sys property level. This improves the global cache config situation as well.

      1. SOLR-6943.patch
        16 kB
        Mark Miller
      2. SOLR-6943.patch
        7 kB
        Mark Miller

        Activity

        Hide
        Mark Miller added a comment -

        Basic patch.

        Show
        Mark Miller added a comment - Basic patch.
        Hide
        Mark Miller added a comment -

        Patch adds testing.

        Show
        Mark Miller added a comment - Patch adds testing.
        Hide
        Mike Drob added a comment -

        My thoughts:

        HdfsDirectoryFactory.java
        +    Integer value = params.getInt(name, defaultValue);
        

        When calling getConfig, for a boolean the precedence is param value, system value, passed default. For strings it is the same order. For ints, it looks like it is param value, passed default, and then system value. Should be consistent with the other two. The problem is on this line.

        HDFSTestUtil.java
        +      Timer timer = new Timer();
        

        Probably outside of the scope of this issue, but using a Timer is sometimes unsafe. Since all Timer objects share a thread, delays or issues in one Timer execution can propogate to other executions (Java Concurrency In Practice, p123). We should consider replacing with a ScheduledThreadPoolExecutor. A follow-on issue is fine for this, I expcet the actual impact to be minimal.

        HdfsDirectoryFactoryTest.java
        +  public void testInitArgsOrSysPropConfig() throws Exception {
        

        This test covers a lot of ground, it would be nice to see it broken down into several smaller tests - one for each scenario that you're trying to do, I think. Not sure if the testing framework is easily amenable to that, however.

        +  public static class MockCoreDescriptor extends CoreDescriptor {
        

        Does this enable something that EasyMock does not?

        +++ solr/core/src/test/org/apache/solr/util/MockSolrResourceLoader.java	(revision 0)
        

        This class looks unused.

        Show
        Mike Drob added a comment - My thoughts: HdfsDirectoryFactory.java + Integer value = params.getInt(name, defaultValue); When calling getConfig , for a boolean the precedence is param value, system value, passed default. For strings it is the same order. For ints, it looks like it is param value, passed default, and then system value. Should be consistent with the other two. The problem is on this line. HDFSTestUtil.java + Timer timer = new Timer(); Probably outside of the scope of this issue, but using a Timer is sometimes unsafe. Since all Timer objects share a thread, delays or issues in one Timer execution can propogate to other executions (Java Concurrency In Practice, p123). We should consider replacing with a ScheduledThreadPoolExecutor . A follow-on issue is fine for this, I expcet the actual impact to be minimal. HdfsDirectoryFactoryTest.java + public void testInitArgsOrSysPropConfig() throws Exception { This test covers a lot of ground, it would be nice to see it broken down into several smaller tests - one for each scenario that you're trying to do, I think. Not sure if the testing framework is easily amenable to that, however. + public static class MockCoreDescriptor extends CoreDescriptor { Does this enable something that EasyMock does not? +++ solr/core/src/test/org/apache/solr/util/MockSolrResourceLoader.java (revision 0) This class looks unused.
        Hide
        Mark Miller added a comment -

        When calling getConfig, for a boolean the precedence is param value, system value, passed default.

        I'm not seeing this.

        Probably outside of the scope of this issue, but using a Timer is sometimes unsafe.

        Right, unrelated to this patch.

        Does this enable something that EasyMock does not?

        Yes, we are avoiding EasyMock for actual easy mocks.

        Show
        Mark Miller added a comment - When calling getConfig, for a boolean the precedence is param value, system value, passed default. I'm not seeing this. Probably outside of the scope of this issue, but using a Timer is sometimes unsafe. Right, unrelated to this patch. Does this enable something that EasyMock does not? Yes, we are avoiding EasyMock for actual easy mocks.
        Hide
        Mark Miller added a comment -

        I'm not seeing this.

        I see - I missed removing the default from the first line in the int one - nice catch.

        Show
        Mark Miller added a comment - I'm not seeing this. I see - I missed removing the default from the first line in the int one - nice catch.
        Hide
        ASF subversion and git services added a comment -

        Commit 1651373 from Mark Miller in branch 'dev/trunk'
        [ https://svn.apache.org/r1651373 ]

        SOLR-6943: HdfsDirectoryFactory should fall back to system props for most of it's config if it is not found in solrconfig.xml.

        Show
        ASF subversion and git services added a comment - Commit 1651373 from Mark Miller in branch 'dev/trunk' [ https://svn.apache.org/r1651373 ] SOLR-6943 : HdfsDirectoryFactory should fall back to system props for most of it's config if it is not found in solrconfig.xml.
        Hide
        ASF subversion and git services added a comment -

        Commit 1651375 from Mark Miller in branch 'dev/branches/branch_5x'
        [ https://svn.apache.org/r1651375 ]

        SOLR-6943: HdfsDirectoryFactory should fall back to system props for most of it's config if it is not found in solrconfig.xml.

        Show
        ASF subversion and git services added a comment - Commit 1651375 from Mark Miller in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1651375 ] SOLR-6943 : HdfsDirectoryFactory should fall back to system props for most of it's config if it is not found in solrconfig.xml.
        Hide
        Anshum Gupta added a comment -

        Bulk close after 5.0 release.

        Show
        Anshum Gupta added a comment - Bulk close after 5.0 release.

          People

          • Assignee:
            Mark Miller
            Reporter:
            Mark Miller
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development