Issue Details (XML | Word | Printable)

Key: HADOOP-4631
Type: Improvement Improvement
Status: Closed Closed
Resolution: Fixed
Priority: Major Major
Assignee: Sharad Agarwal
Reporter: Owen O'Malley
Votes: 0
Watchers: 8
Operations

If you were logged in you would be able to see more operations.
Hadoop Common

Split the default configurations into 3 parts

Created: 11/Nov/08 05:35 AM   Updated: 23/Apr/09 07:17 PM
Return to search
Component/s: conf
Affects Version/s: None
Fix Version/s: 0.20.0

Time Tracking:
Not Specified

File Attachments:
  Size
Text File Licensed for inclusion in ASF works 4631_v1.patch 2008-12-03 08:39 AM Sharad Agarwal 122 kB
Text File Licensed for inclusion in ASF works 4631_v2.patch 2008-12-05 01:19 PM Sharad Agarwal 83 kB
Text File Licensed for inclusion in ASF works 4631_v3.patch 2008-12-08 02:15 PM Sharad Agarwal 139 kB
Text File Licensed for inclusion in ASF works 4631_v4.patch 2008-12-10 12:14 PM Sharad Agarwal 139 kB
Text File Licensed for inclusion in ASF works 4631_v5.patch 2008-12-12 10:25 AM Sharad Agarwal 141 kB
Text File Licensed for inclusion in ASF works 4631_v6.patch 2008-12-12 12:38 PM Sharad Agarwal 143 kB
Text File Licensed for inclusion in ASF works 4631_v7.patch 2008-12-13 05:28 AM Sharad Agarwal 143 kB
Issue Links:
Blocker
 
Incorporates
 
Reference
 

Hadoop Flags: Reviewed, Incompatible change
Release Note: Split hadoop-default.xml into core-default.xml, hdfs-default.xml and mapreduce-default.xml.
Resolution Date: 16/Dec/08 12:05 AM


 Description  « Hide
We need to split hadoop-default.xml into core-default.xml, hdfs-default.xml and mapreduce-default.xml. That will enable us to split the project into 3 parts that have the defaults distributed with each component.

 All   Comments   Work Log   Change History   Subversion Commits      Sort Order: Ascending order - Click to sort in descending order
Doug Cutting added a comment - 11/Nov/08 05:38 PM
All default files should be disjoint, but must also be loaded before hadoop

Here's a proposal:
. add a static 'defaultResources' field to Configuration, whose default value is ["core-default.xml"];
. add a new static method, Configuration#addDefaultResource();
. in JobConf, add 'static { Configuration.addDefaultResource("mapred-default,xml"); }'
. in DistributedFileSystem.java, NameNode.java and DataNode.java, add 'static { Configuration.addDefaultResource("hdfs-default,xml"); }' We might factor this to a common base class or somesuch.

When you first call FileSystem.get("hdfs:///", conf), your configuration won't yet have hdfs-specific default values, but, in the course of this call, they will be loaded, before any hdfs code references them. I think this is fine, since application code should not be directly referencing hdfs-specific values. If it needs to set them, it should do so through accessor methods, and these accessor methods should dereference an HDFS class and force the loading of the HDFS defaults. Does that make sense?


Doug Cutting added a comment - 11/Nov/08 05:43 PM
To avoid confusion, default config files should come from the jar file, not from a conf directory. So we should never commit these new default files to conf/, but rather start with them in src/, copied to build/classes and from there into the jar. Thus this issue will also fix HADOOP-4617, no?

Sharad Agarwal added a comment - 25/Nov/08 09:48 AM

When you first call FileSystem.get("hdfs:///", conf), your configuration won't yet have hdfs-specific default values, but, in the course of this call, they will be loaded, before any hdfs code references them. I think this is fine, since application code should not be directly referencing hdfs-specific values.

For the Configuration objects created prior to loading of HDFS defaults, the properties won't be reloaded. Currently, Configuration objects are created and same being passed around. These objects if used to load hdfs specific values, they won't be able to. no?
For example: DistributedFileSystem uses the Configuration object created prior to loading of itself.


Sharad Agarwal added a comment - 25/Nov/08 11:33 AM
I am thinking that instead of keeping defaults in static member, will it make sense if we keep it in specific instances. This would separate things clearly; though may require more changes.
JobConf will do addResouce(mapred-default.xml) in its constructor and similarly we can create HDFSConf (extends Configuration) which will do addResource(hdfs-default.xml)
The hdfs code would instantiate HDFSConf instead of Configuration. If Configuration object is passed to it for example to DistributedFileSystem, then it would wrap it in HDFSConf before looking for hdfs specific values.

Doug Cutting added a comment - 25/Nov/08 07:14 PM
> For the Configuration objects created prior to loading of HDFS defaults, the properties won't be reloaded.

Hmm. I guess you're right. We could keep a static WeakHashMap listing all existing Configuration instances, and cause them to be reloaded when the list of defaults changes?

> we can create HDFSConf (extends Configuration) [ ... ]

That's a pattern we'd like to get away from. The problem is that applications need to be able configure HDFS, MapReduce, Pig, etc., all with a single Configuration instance. Look, for example at the next-generation MapReduce API in:

http://svn.apache.org/repos/asf/hadoop/core/trunk/src/mapred/org/apache/hadoop/mapreduce/Job.java


Sharad Agarwal added a comment - 03/Dec/08 08:39 AM
this patch:
Replaces the hadoop-default|site.xml with core-default|site.xml, mapred-default|site.xml and hdfs-default|site.xml
Keeps the registry of configuration objects in Configuration. Whenever ad new default resource is added, all configuration objects are reloaded

TODO:
move the *-default to src
fix all the documentation related hadoop-default.xml//hadoop-site.xml
fix the code in contrib which uses hadoop-default.xml/hadoop-site.xml


Doug Cutting added a comment - 04/Dec/08 06:15 PM
If we want to get this into 0.20, let's file a separate issue for the documentation, as documentation changes can be made after the feature freeze tomorrow. It would be good to get this into 0.20, no?

Sharad Agarwal added a comment - 05/Dec/08 10:07 AM

It would be good to get this into 0.20, no?

Apart from documentation, I think there are few things which needs to ironed out before this gets into:

  • backward compatibility: should we keep hadoop-site.xml as well in conf and add it in Configuration class as a default resource before core-site.xml ? It should be sufficient for the backward compatibility, right? Or keeping it will add to the confusion, lets live with the incompatible change ?
    hadoop-default.xml, anyways people should not care about, so we don't need to keep it. Moving of defaults to src folder should not have any impact on applications.
  • Applications can provide conf file via bin/hadoop -conf option. The provided conf file should override the defaults. In the current code, if hdfs or mapred code is linked later on, the default resource are added in the end of the Configuration#resources . This would make default override the user provided config file, which is not correct. We can do :
    private void loadResources(Properties properties,
                                 ArrayList resources,
                                 boolean quiet) {
        if(loadDefaults) {
          for (String resource : defaultResources) {
            loadResource(properties, resource, quiet);
          }
        }
        for (Object resource : resources) {
          loadResource(properties, resource, quiet);
        }
      }
  • currently contrib projects like hod generates the hadoop-site.xml and uses it in various scripts. I am not sure what impact this jira have on such projects. Contrib test cases are very limited and not sufficient to catch problems. So wherever hadoop-site.xml is being referred in the code has to be analyzed closely.

Sharad Agarwal added a comment - 05/Dec/08 01:19 PM
upated to trunk.
moved *-default.xml files to respective src folder
fixed default resource loading as mentioned in above comment
made minor changes in failmon and chukwa

Doug Cutting added a comment - 05/Dec/08 05:51 PM
Back-compatibility is a concern. I think we should still always look for and load hadoop-site.xml but not hadoop-default.xml. When we find and load hadoop-site.xml we should emit a warning.

Sharad Agarwal added a comment - 08/Dec/08 02:15 PM
Attaching the patch for review.
  • supported backward compatibility. If hadoop-site.xml is found in classpath it is loaded, emitting a warning.
  • documentation updation will be done in a separate issue.
  • all the core tests are passing on my machine.

Sharad Agarwal added a comment - 10/Dec/08 12:14 PM
updated to trunk and fixed a javadoc warning.

Sharad Agarwal added a comment - 12/Dec/08 10:25 AM
updated to trunk

Hemanth Yamijala added a comment - 12/Dec/08 10:39 AM

supported backward compatibility. If hadoop-site.xml is found in classpath it is loaded, emitting a warning.

To give some time (read - past this release) for clients like HOD which may be reasonably impacted by this change, can we avoid this warning from becoming too verbose ? That is, instead of printing it everytime a configuration object is loaded, can we emit it only once ?


Devaraj Das added a comment - 12/Dec/08 11:22 AM
Some minor nits:
1) Move the property dfs.block.replication from src/c+/libhdfs/tests/conf/core-site.xml to src/c+/libhdfs/tests/conf/hdfs-site.xml
2) Have a static loading for the conf resources in JobTracker and TaskTracker as well

Sharad Agarwal added a comment - 12/Dec/08 12:38 PM
Incorporated feedback by Hemanth and Devaraj.
All tests and test-patch passed.

Devaraj Das added a comment - 12/Dec/08 05:29 PM
I just committed this. Thanks, Sharad! (Please add a Release Note)

Tsz Wo (Nicholas), SZE added a comment - 12/Dec/08 06:47 PM - edited
The forrest doc should be updated.

Tsz Wo (Nicholas), SZE added a comment - 12/Dec/08 08:05 PM
These tests seem failing after the patch:
    [junit] Running org.apache.hadoop.fs.TestCopyFiles
    [junit] Tests run: 12, Failures: 2, Errors: 8, Time elapsed: 164.049 sec
    [junit] Running org.apache.hadoop.fs.TestDFSIO
    [junit] Tests run: 1, Failures: 0, Errors: 1, Time elapsed: 10.468 sec
...
    [junit] Running org.apache.hadoop.fs.TestFileSystem
    [junit] Tests run: 5, Failures: 0, Errors: 2, Time elapsed: 20.726 sec
    [junit] Running org.apache.hadoop.fs.TestGetFileBlockLocations
    [junit] Tests run: 3, Failures: 0, Errors: 3, Time elapsed: 30.824 sec
...
    [junit] Running org.apache.hadoop.hdfs.TestDFSShell
    [junit] Tests run: 14, Failures: 1, Errors: 0, Time elapsed: 93.783 sec
 

Chris Douglas added a comment - 12/Dec/08 08:12 PM
I reverted this. The test cases fail with the patch applied, and pass when it's reverted

Chris Douglas added a comment - 12/Dec/08 08:54 PM
The svn and git ignore lists should be updated as part of this patch, too.

Sharad Agarwal added a comment - 13/Dec/08 05:01 AM
The tests are passing on my linux machine with the patch. Also verified with pre-reverted Revision: 726075

The problem could be user hadoop-site.xml overriding the test properties. With this patch, src/test/hadoop-site.xml is removed and respective properties are moved to core-site.xml, hdfs-site.xml and mapred-site.xml.

To overcome this problem, we can keep empty src/test/hadoop-site.xml so that it overrides the conf/hadoop-site.xml and user properties are not picked up while executing test cases.


Sharad Agarwal added a comment - 13/Dec/08 05:28 AM
changes from previous patch:
  • Instread of removing, keeps empty:
    src/test/hadoop-site.xml
    src/contrib/test/hadoop-site.xml
    src/c++/libhdfs/tests/conf/hadoop-site.xml
  • adds conf/core-site.xml, conf/hdfs-site.xml and conf/mapred-site.xml to .gitignore

Sharad Agarwal added a comment - 13/Dec/08 08:02 AM
all tests and test-patch passed.

Chris Douglas added a comment - 13/Dec/08 09:21 AM
I can no longer reproduce the test failures, not even after merging the original changes back in. Sharad's theory sounds like the most likely explanation.

Nicholas, can you reproduce the failures you were seeing earlier?


Tsz Wo (Nicholas), SZE added a comment - 15/Dec/08 07:45 PM
     [exec] +1 overall.  

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

     [exec]     +1 tests included.  The patch appears to include 51 new or modified tests.

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

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

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

     [exec]     +1 Eclipse classpath. The patch retains Eclipse classpath integrity.

Running unit tests locally.


Tsz Wo (Nicholas), SZE added a comment - 15/Dec/08 09:26 PM
Only the following tests failed. I think they are not related to this patch.

Tsz Wo (Nicholas), SZE added a comment - 16/Dec/08 12:04 AM
+1 for committing this.

Chris Douglas added a comment - 16/Dec/08 12:05 AM
I just (re)committed this. Thanks Sharad

Robert Chansler added a comment - 03/Mar/09 06:32 PM
Edit release note for publication.