Hadoop Common
  1. Hadoop Common
  2. HADOOP-4365

Configuration.getProps() should be made protected for ease of overriding

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.21.0
    • Fix Version/s: 0.21.0
    • Component/s: conf
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      HADOOP-4293 does make Configuration cleaner and easier to work with, but it does contain assumptions about how configurations are represented (in a private Properties instance) that subclasses may wish to diverge from.

      By making getProps() protected, people who subclass JobConf or Configuration can do their own binding from configuration data to Properties.

      1. hadoop-4365.patch
        0.5 kB
        Steve Loughran
      2. hadoop-4365.patch
        4 kB
        Steve Loughran

        Activity

        Hide
        Doug Cutting added a comment -

        JobConf will soon go away, with HADOOP-1230.

        That said, letting folks override getProps() sounds fine. This revives the spirit of HADOOP-24.

        Show
        Doug Cutting added a comment - JobConf will soon go away, with HADOOP-1230 . That said, letting folks override getProps() sounds fine. This revives the spirit of HADOOP-24 .
        Hide
        Steve Loughran added a comment -

        -we have subclassed Configuration to bind to a different configuration source; JobConf is subclassed because so many things expect it. The reason I need to access/override getProps() is so that when Configuration tries to enum all property tuples, I need to generate that list, and so let the base class serialize the state and share it.

        I'll provide the patch for this.

        Show
        Steve Loughran added a comment - -we have subclassed Configuration to bind to a different configuration source; JobConf is subclassed because so many things expect it. The reason I need to access/override getProps() is so that when Configuration tries to enum all property tuples, I need to generate that list, and so let the base class serialize the state and share it. I'll provide the patch for this.
        Hide
        Steve Loughran added a comment -

        Here's the patch for this. No tests; it just changes the protection level of one method

        Show
        Steve Loughran added a comment - Here's the patch for this. No tests; it just changes the protection level of one method
        Hide
        Doug Cutting added a comment -

        This needs some javadoc, but other than that seems fine.

        Longer-term, should we replace 'new Configuration()' with a factory API? Would that help you at all? Or does that just move the problem?

        Show
        Doug Cutting added a comment - This needs some javadoc, but other than that seems fine. Longer-term, should we replace 'new Configuration()' with a factory API? Would that help you at all? Or does that just move the problem?
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12395566/hadoop-4365.patch
        against trunk revision 725547.

        +1 @author. The patch does not contain any @author tags.

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no tests are needed for this patch.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 findbugs. The patch does not introduce any new Findbugs warnings.

        +1 Eclipse classpath. The patch retains Eclipse classpath integrity.

        +1 core tests. The patch passed core unit tests.

        +1 contrib tests. The patch passed contrib unit tests.

        Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3705/testReport/
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3705/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3705/artifact/trunk/build/test/checkstyle-errors.html
        Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3705/console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12395566/hadoop-4365.patch against trunk revision 725547. +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no tests are needed for this patch. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 Eclipse classpath. The patch retains Eclipse classpath integrity. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3705/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3705/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3705/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3705/console This message is automatically generated.
        Hide
        Steve Loughran added a comment -

        I will add a test for this; catches regressions

        Show
        Steve Loughran added a comment - I will add a test for this; catches regressions
        Hide
        Steve Loughran added a comment -

        This patch adds tests that

        • the Configuration.getProperties() method is accessible from subclasses
        • if you add a new default property, configuration files get their reloadConfiguration() method called.
        • if you call Configuration.setQuietMode(true), then getProperties() is one place where a load error is raised.

        There's a new empty configuration xml file added as default, it cannot be unloaded until HADOOP-5093 is fixed.

        Show
        Steve Loughran added a comment - This patch adds tests that the Configuration.getProperties() method is accessible from subclasses if you add a new default property, configuration files get their reloadConfiguration() method called. if you call Configuration.setQuietMode(true), then getProperties() is one place where a load error is raised. There's a new empty configuration xml file added as default, it cannot be unloaded until HADOOP-5093 is fixed.
        Hide
        Chris Douglas added a comment -

        +1

        I committed this. Thanks, Steve

        Show
        Chris Douglas added a comment - +1 I committed this. Thanks, Steve
        Hide
        Hudson added a comment -
        Show
        Hudson added a comment - Integrated in Hadoop-trunk #796 (See http://hudson.zones.apache.org/hudson/job/Hadoop-trunk/796/ )

          People

          • Assignee:
            Steve Loughran
            Reporter:
            Steve Loughran
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development