Hadoop Common
  1. Hadoop Common
  2. HADOOP-6408

Add a /conf servlet to dump running configuration

    Details

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

      Description

      HADOOP-6184 added a command line flag to dump the running configuration. It would be great for cluster troubleshooting to provide access to this as a servlet, preferably in both JSON and XML formats. But really, any format would be better than nothing. This should/could go into all of the daemons.

      1. hadoop-6408.txt
        9 kB
        Todd Lipcon
      2. hadoop-6408.txt
        11 kB
        Todd Lipcon
      3. hadoop-6408.txt
        16 kB
        Todd Lipcon
      4. hadoop-6408.txt
        15 kB
        Todd Lipcon
      5. hadoop-6408.txt
        17 kB
        Todd Lipcon
      6. hadoop-6408.txt
        18 kB
        Todd Lipcon
      7. hadoop-6408.txt
        19 kB
        Todd Lipcon
      8. hadoop-6408.txt
        19 kB
        Todd Lipcon

        Issue Links

          Activity

          Hide
          Henry Robinson added a comment -

          This would be super useful. Can it also tell us which resources the conf has actually been loaded from, if that information's recoverable?

          Show
          Henry Robinson added a comment - This would be super useful. Can it also tell us which resources the conf has actually been loaded from, if that information's recoverable?
          Hide
          Todd Lipcon added a comment -

          Yep, that info is recoverable and included in the JSON output from HADOOP-6184.

          It's not included in the XML output.

          I'll also probably add an HTML tabular output and include it there, if I have time.

          Show
          Todd Lipcon added a comment - Yep, that info is recoverable and included in the JSON output from HADOOP-6184 . It's not included in the XML output. I'll also probably add an HTML tabular output and include it there, if I have time.
          Hide
          Todd Lipcon added a comment -

          This patch implements a servlet at /conf that dumps the configuration. ?format=xml and ?format=json are provided (xml is the default).

          This also modifies the .writeXml() function of Configuration to include the resource sources (when available) as xml comments. Eg:

          <property>
            <!--Loaded from core-default.xml-->
            <name>fs.s3n.impl</name>
            <value>org.apache.hadoop.fs.s3native.NativeS3FileSystem</value>
          </property>
          

          I did some slight refactoring there to make things more convenient.

          I didn't do an HTML view of this data, since Firefox at least displays the XML pretty readably. If others would find HTML useful, I'd like to do that in a separate JIRA.

          Show
          Todd Lipcon added a comment - This patch implements a servlet at /conf that dumps the configuration. ?format=xml and ?format=json are provided (xml is the default). This also modifies the .writeXml() function of Configuration to include the resource sources (when available) as xml comments. Eg: <property> <!--Loaded from core-default.xml--> <name>fs.s3n.impl</name> <value>org.apache.hadoop.fs.s3native.NativeS3FileSystem</value> </property> I did some slight refactoring there to make things more convenient. I didn't do an HTML view of this data, since Firefox at least displays the XML pretty readably. If others would find HTML useful, I'd like to do that in a separate JIRA.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12426712/hadoop-6408.txt
          against trunk revision 886003.

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

          +1 tests included. The patch appears to include 2 new or modified tests.

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

          -1 javac. The applied patch generated 173 javac compiler warnings (more than the trunk's current 172 warnings).

          -1 findbugs. The patch appears to introduce 2 new Findbugs warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +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-h4.grid.sp2.yahoo.net/159/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/159/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/159/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/159/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/12426712/hadoop-6408.txt against trunk revision 886003. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 2 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. -1 javac. The applied patch generated 173 javac compiler warnings (more than the trunk's current 172 warnings). -1 findbugs. The patch appears to introduce 2 new Findbugs warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +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-h4.grid.sp2.yahoo.net/159/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/159/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/159/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/159/console This message is automatically generated.
          Hide
          Todd Lipcon added a comment -

          New patch to address findbugs and javac warnings:

          • Added a serialVersionUID to ConfServlet (apparently Servlets are serializable)
          • Added some more synchronized blocks in Configuration - was missing some synchronization while iterating over properties, etc, which was exposed as a side effect of adding some code.

          The other findbugs warning was about the catch (Exception) in writeXml, which already existed. I'd be happy to explicitly catch the thrown XML-related exceptions and rethrow as an IOException, but I didn't write that patch and didn't want to change it if it were purposefully a RuntimeException. Thoughts?

          Show
          Todd Lipcon added a comment - New patch to address findbugs and javac warnings: Added a serialVersionUID to ConfServlet (apparently Servlets are serializable) Added some more synchronized blocks in Configuration - was missing some synchronization while iterating over properties, etc, which was exposed as a side effect of adding some code. The other findbugs warning was about the catch (Exception) in writeXml, which already existed. I'd be happy to explicitly catch the thrown XML-related exceptions and rethrow as an IOException, but I didn't write that patch and didn't want to change it if it were purposefully a RuntimeException. Thoughts?
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12426723/hadoop-6408.txt
          against trunk revision 886645.

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

          +1 tests included. The patch appears to include 2 new or modified tests.

          +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 appears to introduce 1 new Findbugs warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +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-h4.grid.sp2.yahoo.net/160/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/160/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/160/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/160/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/12426723/hadoop-6408.txt against trunk revision 886645. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 2 new or modified tests. +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 appears to introduce 1 new Findbugs warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +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-h4.grid.sp2.yahoo.net/160/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/160/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/160/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/160/console This message is automatically generated.
          Hide
          Todd Lipcon added a comment -

          -1 findbugs. The patch appears to introduce 1 new Findbugs warnings.

          Not sure why this is considered new, since that code existed beforehand. See above. Aside from that, think this is good for review.

          Show
          Todd Lipcon added a comment - -1 findbugs. The patch appears to introduce 1 new Findbugs warnings. Not sure why this is considered new, since that code existed beforehand. See above. Aside from that, think this is good for review.
          Hide
          steve_l added a comment -

          -you don't need to generate an HTML version if you include a URL to a locally served XSL file, IE and FF will use the style sheet.
          -Alternatively, consider some XHTML

          Or don't bother, just have something that is easy to attach to bug reports.

          One worry: security. Does anyone put passwords in their configurations? If so, this would expose it. Accordingly, it may be useful to make this a feature which can be disabled.

          Show
          steve_l added a comment - -you don't need to generate an HTML version if you include a URL to a locally served XSL file, IE and FF will use the style sheet. -Alternatively, consider some XHTML Or don't bother, just have something that is easy to attach to bug reports. One worry: security. Does anyone put passwords in their configurations? If so, this would expose it. Accordingly, it may be useful to make this a feature which can be disabled.
          Hide
          Todd Lipcon added a comment -

          Fair enough regarding HTML/XHTML.

          The bug report thing is one of the primary motivators - the other is in our own use when supporting clients.

          Regarding security, the only place I can think of where something private would be ni the daemon confs would be S3 access keys. However, putting those in the daemons is useless since the daemons themselves don't need to talk to ec2.

          If you think it's important, though, happy to oblige. I certainly could have missed another piece of private data.

          -Todd

          Show
          Todd Lipcon added a comment - Fair enough regarding HTML/XHTML. The bug report thing is one of the primary motivators - the other is in our own use when supporting clients. Regarding security, the only place I can think of where something private would be ni the daemon confs would be S3 access keys. However, putting those in the daemons is useless since the daemons themselves don't need to talk to ec2. If you think it's important, though, happy to oblige. I certainly could have missed another piece of private data. -Todd
          Hide
          Todd Lipcon added a comment -

          cancelling patch to add the disable feature. Also realized that it's loading the configuration fresh off disk, not displaying what's actually running, which makes it a little less useful.

          Show
          Todd Lipcon added a comment - cancelling patch to add the disable feature. Also realized that it's loading the configuration fresh off disk, not displaying what's actually running, which makes it a little less useful.
          Hide
          Todd Lipcon added a comment -

          Steve: it looks like there's already hadoop.http.filter.initializers which lets you insert arbitrary filters in front of the user-facing servlets. Since /conf has those filters put in front, this would make it possible to restrict access to it. I imagine that's the use case for this configuration (though there are no implementations to speak of that I can find).

          Would you still prefer that I add a conf explicitly for this servlet?

          Show
          Todd Lipcon added a comment - Steve: it looks like there's already hadoop.http.filter.initializers which lets you insert arbitrary filters in front of the user-facing servlets. Since /conf has those filters put in front, this would make it possible to restrict access to it. I imagine that's the use case for this configuration (though there are no implementations to speak of that I can find). Would you still prefer that I add a conf explicitly for this servlet?
          Hide
          Todd Lipcon added a comment -

          New patch with the following changes:

          • Configuration now always tracks the "setting resource". The reason for this change is that, otherwise, it's impossible to dump the "running configuration" of the server without reloading from disk. If you reload from disk, you aren't seeing the configuration that's actually governing the server's behavior, so the servlet's less than useful.
          • I don't believe this should be a performance issue, since it only affects Configuration.set() and other mutators. These calls are not made frequently. Also, the number of Configuration objects floating around a daemon is reasonably small compared to its overall RAM usage, so I don't think the memory overhead will be a problem.

          I did not implement access control for the reasons I provided above. Confs are already leaked in the web UI in the form of job.xml files, and people can use the Filter mechanism otherwise.

          Show
          Todd Lipcon added a comment - New patch with the following changes: Configuration now always tracks the "setting resource". The reason for this change is that, otherwise, it's impossible to dump the "running configuration" of the server without reloading from disk. If you reload from disk, you aren't seeing the configuration that's actually governing the server's behavior, so the servlet's less than useful. I don't believe this should be a performance issue, since it only affects Configuration.set() and other mutators. These calls are not made frequently. Also, the number of Configuration objects floating around a daemon is reasonably small compared to its overall RAM usage, so I don't think the memory overhead will be a problem. I did not implement access control for the reasons I provided above. Confs are already leaked in the web UI in the form of job.xml files, and people can use the Filter mechanism otherwise.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12427518/hadoop-6408.txt
          against trunk revision 888697.

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

          +1 tests included. The patch appears to include 2 new or modified tests.

          +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 appears to introduce 2 new Findbugs warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +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-h1.grid.sp2.yahoo.net/24/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h1.grid.sp2.yahoo.net/24/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h1.grid.sp2.yahoo.net/24/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h1.grid.sp2.yahoo.net/24/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/12427518/hadoop-6408.txt against trunk revision 888697. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 2 new or modified tests. +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 appears to introduce 2 new Findbugs warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +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-h1.grid.sp2.yahoo.net/24/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h1.grid.sp2.yahoo.net/24/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h1.grid.sp2.yahoo.net/24/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h1.grid.sp2.yahoo.net/24/console This message is automatically generated.
          Hide
          Todd Lipcon added a comment -

          Findbugs pointed out that the conf member of ConfServlet wasn't defined as transient, which made me do some research and learn that in some rare circumstances, HttpServlets can get serialized. The recommended place to put state like this is in the ServletContext.

          This new patch stores Configuration in ServletContext. It's a bit simpler patch, too, with fewer changes to HttpServer.

          It's tested by unit tests, and I also tested manually by running a namenode built against this and visually inspecting /conf there.

          Show
          Todd Lipcon added a comment - Findbugs pointed out that the conf member of ConfServlet wasn't defined as transient, which made me do some research and learn that in some rare circumstances, HttpServlets can get serialized. The recommended place to put state like this is in the ServletContext. This new patch stores Configuration in ServletContext. It's a bit simpler patch, too, with fewer changes to HttpServer. It's tested by unit tests, and I also tested manually by running a namenode built against this and visually inspecting /conf there.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12427525/hadoop-6408.txt
          against trunk revision 888998.

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

          +1 tests included. The patch appears to include 2 new or modified tests.

          +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 appears to introduce 1 new Findbugs warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +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-h4.grid.sp2.yahoo.net/182/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/182/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/182/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/182/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/12427525/hadoop-6408.txt against trunk revision 888998. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 2 new or modified tests. +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 appears to introduce 1 new Findbugs warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +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-h4.grid.sp2.yahoo.net/182/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/182/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/182/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/182/console This message is automatically generated.
          Hide
          Todd Lipcon added a comment -

          -1 findbugs. The patch appears to introduce 1 new Findbugs warnings.

          This is actually an old FindBugs warning - I simply changed the signature of the writeXml method to take a Writer instead of an OutputStream, so it thinks it's new.

          This should be ready for human review.

          Show
          Todd Lipcon added a comment - -1 findbugs. The patch appears to introduce 1 new Findbugs warnings. This is actually an old FindBugs warning - I simply changed the signature of the writeXml method to take a Writer instead of an OutputStream, so it thinks it's new. This should be ready for human review.
          Hide
          steve_l added a comment -

          Servlet serialization is a funny, from the world of shared-state front-end machines; servlet state can be pushed into some common space so the same servlet re-used. Normally it exists to stop dynamic webapp reload working under Tomcat and friends.

          Given that configurations don't normally contain security-sensitive data, we could just ignore the problem here, or possibly consider adding the option to disable this servlet if you really don't want it. That's much simpler to handle than filtering content.

          Show
          steve_l added a comment - Servlet serialization is a funny, from the world of shared-state front-end machines; servlet state can be pushed into some common space so the same servlet re-used. Normally it exists to stop dynamic webapp reload working under Tomcat and friends. Given that configurations don't normally contain security-sensitive data, we could just ignore the problem here, or possibly consider adding the option to disable this servlet if you really don't want it. That's much simpler to handle than filtering content.
          Hide
          Todd Lipcon added a comment -

          That's much simpler to handle than filtering content.

          I may be incorrect, but I believe the filter could be as simple as "Show 404 Forbidden for /conf". Shouldn't be more than a few lines of code to do that. So, I'm in favor of ignoring the security issue and if people have an issue we can provide some sample code for that filter. As the web apps gain proper security in 22, this will be protected just the same.

          Show
          Todd Lipcon added a comment - That's much simpler to handle than filtering content. I may be incorrect, but I believe the filter could be as simple as "Show 404 Forbidden for /conf". Shouldn't be more than a few lines of code to do that. So, I'm in favor of ignoring the security issue and if people have an issue we can provide some sample code for that filter. As the web apps gain proper security in 22, this will be protected just the same.
          Hide
          Tom White added a comment -

          +1 Looks good to me.

          A couple of minor comments:

          • It would be nice to make the string literals in ConfServlet constants.
          • Make TestConfServlet check that the output is as expected, rather than just non-null.
          Show
          Tom White added a comment - +1 Looks good to me. A couple of minor comments: It would be nice to make the string literals in ConfServlet constants. Make TestConfServlet check that the output is as expected, rather than just non-null.
          Hide
          Todd Lipcon added a comment -

          Thanks for the review, Tom. This new patch adds constants for the strings, and the unit tests now actually verify that the output contains actual an actual key/val.

          Show
          Todd Lipcon added a comment - Thanks for the review, Tom. This new patch adds constants for the strings, and the unit tests now actually verify that the output contains actual an actual key/val.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12428995/hadoop-6408.txt
          against trunk revision 893666.

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

          +1 tests included. The patch appears to include 2 new or modified tests.

          +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 appears to introduce 1 new Findbugs warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +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-h4.grid.sp2.yahoo.net/241/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/241/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/241/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/241/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/12428995/hadoop-6408.txt against trunk revision 893666. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 2 new or modified tests. +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 appears to introduce 1 new Findbugs warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +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-h4.grid.sp2.yahoo.net/241/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/241/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/241/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/241/console This message is automatically generated.
          Hide
          Tom White added a comment -
          • The catch block in writeXml() can actually be removed without changing behaviour, since the method already declares that it throws IOException, and any RuntimeException is re-thrown. This would remove the FindBugs warning.
          • The javadoc on the writeXml(Writer) method is incorrect since it refers to OutputStream.
          Show
          Tom White added a comment - The catch block in writeXml() can actually be removed without changing behaviour, since the method already declares that it throws IOException, and any RuntimeException is re-thrown. This would remove the FindBugs warning. The javadoc on the writeXml(Writer) method is incorrect since it refers to OutputStream.
          Hide
          Todd Lipcon added a comment -

          New patch in response to Tom's review:

          • I couldn't remove the try/catch block entirely, as the XML code throws two non-IOException checked exceptions. Instead, I changed it to explicitly catch those and rethrow them as IOException, which is probably more appropriate than RTE.
          • Fixed javadoc on writeXml
          Show
          Todd Lipcon added a comment - New patch in response to Tom's review: I couldn't remove the try/catch block entirely, as the XML code throws two non-IOException checked exceptions. Instead, I changed it to explicitly catch those and rethrow them as IOException, which is probably more appropriate than RTE. Fixed javadoc on writeXml
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12429288/hadoop-6408.txt
          against trunk revision 894706.

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

          +1 tests included. The patch appears to include 2 new or modified tests.

          +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 release audit. The applied patch does not increase the total number of release audit warnings.

          +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-h4.grid.sp2.yahoo.net/248/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/248/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/248/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/248/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/12429288/hadoop-6408.txt against trunk revision 894706. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 2 new or modified tests. +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 release audit. The applied patch does not increase the total number of release audit warnings. +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-h4.grid.sp2.yahoo.net/248/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/248/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/248/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/248/console This message is automatically generated.
          Hide
          steve_l added a comment -

          Looks pretty good. If there is one change I'd do it is have an unrecognised format generate an HTTP 400 response (bad request) when an unknown format is hit; then add a test for this. That way automated clients asking for formats get to fail without trying to parse the (presumably invalid) text

          Show
          steve_l added a comment - Looks pretty good. If there is one change I'd do it is have an unrecognised format generate an HTTP 400 response (bad request) when an unknown format is hit; then add a test for this. That way automated clients asking for formats get to fail without trying to parse the (presumably invalid) text
          Hide
          Todd Lipcon added a comment -

          New patch in response to Steve's comments:

          • Now sets HTTP content type header appropriately
          • Sets status code to 400 for bad format.

          Tested by unit tests, as well as manually using the namenode. I verified JSON output, XML output, and a bad format parameter. Also tested putting < and > in the format parameter to see if there was an XSS reflection attack, and it was properly escaped in the error message.

          Show
          Todd Lipcon added a comment - New patch in response to Steve's comments: Now sets HTTP content type header appropriately Sets status code to 400 for bad format. Tested by unit tests, as well as manually using the namenode. I verified JSON output, XML output, and a bad format parameter. Also tested putting < and > in the format parameter to see if there was an XSS reflection attack, and it was properly escaped in the error message.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12429480/hadoop-6408.txt
          against trunk revision 896259.

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

          +1 tests included. The patch appears to include 2 new or modified tests.

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

          -1 javac. The applied patch generated 162 javac compiler warnings (more than the trunk's current 161 warnings).

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

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +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-h4.grid.sp2.yahoo.net/255/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/255/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/255/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/255/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/12429480/hadoop-6408.txt against trunk revision 896259. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 2 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. -1 javac. The applied patch generated 162 javac compiler warnings (more than the trunk's current 161 warnings). +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +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-h4.grid.sp2.yahoo.net/255/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/255/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/255/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/255/console This message is automatically generated.
          Hide
          Todd Lipcon added a comment -

          fix javac warning (missing serialVersionUID in new inner Exception)

          Show
          Todd Lipcon added a comment - fix javac warning (missing serialVersionUID in new inner Exception)
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12429485/hadoop-6408.txt
          against trunk revision 896259.

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

          +1 tests included. The patch appears to include 2 new or modified tests.

          +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 release audit. The applied patch does not increase the total number of release audit warnings.

          +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-h4.grid.sp2.yahoo.net/256/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/256/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/256/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/256/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/12429485/hadoop-6408.txt against trunk revision 896259. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 2 new or modified tests. +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 release audit. The applied patch does not increase the total number of release audit warnings. +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-h4.grid.sp2.yahoo.net/256/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/256/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/256/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/256/console This message is automatically generated.
          Hide
          steve_l added a comment -

          Looks good to me, nice tests. I'm not sure you are meant to use 1L for a serial version uid, but then it's only to shut the compiler up.

          +1

          Show
          steve_l added a comment - Looks good to me, nice tests. I'm not sure you are meant to use 1L for a serial version uid, but then it's only to shut the compiler up. +1
          Hide
          Tom White added a comment -

          I've just committed this. Thanks Todd!

          Show
          Tom White added a comment - I've just committed this. Thanks Todd!
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk-Commit #135 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Common-trunk-Commit/135/)
          . Add a /conf servlet to dump running configuration. Contributed by Todd Lipcon.

          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk-Commit #135 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Common-trunk-Commit/135/ ) . Add a /conf servlet to dump running configuration. Contributed by Todd Lipcon.
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk #211 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Common-trunk/211/)
          . Add a /conf servlet to dump running configuration. Contributed by Todd Lipcon.

          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk #211 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Common-trunk/211/ ) . Add a /conf servlet to dump running configuration. Contributed by Todd Lipcon.
          Hide
          Hemanth Yamijala added a comment -

          I apologize for not coming in earlier on this. But I have a concern with one particular point that was implemented in this patch:

          Configuration now always tracks the "setting resource".

          and

          Also, the number of Configuration objects floating around a daemon is reasonably small compared to its overall RAM usage, so I don't think the memory overhead will be a problem.

          In Map/Reduce, please note that every JobInProgress holds a reference to a JobConf. Since each JobConf will cause a load of the default resources, a duplicate hash map that manages the mapping between key and the setting resource will now be held for every Job held in memory. I believe this will be a significant memory overload particularly if the job load is very high. It was for this reason that we had chosen the approach we took in HADOOP-6184 to enable this map only on demand.

          Does this make sense ?

          Show
          Hemanth Yamijala added a comment - I apologize for not coming in earlier on this. But I have a concern with one particular point that was implemented in this patch: Configuration now always tracks the "setting resource". and Also, the number of Configuration objects floating around a daemon is reasonably small compared to its overall RAM usage, so I don't think the memory overhead will be a problem. In Map/Reduce, please note that every JobInProgress holds a reference to a JobConf. Since each JobConf will cause a load of the default resources, a duplicate hash map that manages the mapping between key and the setting resource will now be held for every Job held in memory. I believe this will be a significant memory overload particularly if the job load is very high. It was for this reason that we had chosen the approach we took in HADOOP-6184 to enable this map only on demand. Does this make sense ?
          Hide
          Todd Lipcon added a comment -

          Hi Hemanth,

          Yes, I understood that was the original intention, but I don't see how the memory usage will be very large when compared with the rest of the JobInProgress object:

          In particular, this map consists mostly of flyweight references. The keys of the map are configuration keys, which are references to String objects already stored by Configuration. The values are Strings which are created once per resource that's loaded. So, the hashmap's memory footprint doesn't double the footprint of the Configuration object by any means - it just has the footprint of the references themselves.

          A moderately full JobConf probably has a couple hundred configuration parameters, and a fairly full JT has a couple hundred jobs. If each JobConf takes an additional 32 bytes per key (16 bytes for the references, and 16 bytes worth of hashmap overhead) then we should be talking a few KB per JobConf, and 5-10MB overall on the JobTracker. To me that seems like a pretty small cost, as any JT that's managing hundreds of jobs probably has many GB of RAM.

          If necessary, I'm happy to write a quick test to measure the memory usage of Configuration with and without the change.

          Show
          Todd Lipcon added a comment - Hi Hemanth, Yes, I understood that was the original intention, but I don't see how the memory usage will be very large when compared with the rest of the JobInProgress object: In particular, this map consists mostly of flyweight references. The keys of the map are configuration keys, which are references to String objects already stored by Configuration. The values are Strings which are created once per resource that's loaded. So, the hashmap's memory footprint doesn't double the footprint of the Configuration object by any means - it just has the footprint of the references themselves. A moderately full JobConf probably has a couple hundred configuration parameters, and a fairly full JT has a couple hundred jobs. If each JobConf takes an additional 32 bytes per key (16 bytes for the references, and 16 bytes worth of hashmap overhead) then we should be talking a few KB per JobConf, and 5-10MB overall on the JobTracker. To me that seems like a pretty small cost, as any JT that's managing hundreds of jobs probably has many GB of RAM. If necessary, I'm happy to write a quick test to measure the memory usage of Configuration with and without the change.
          Hide
          Hemanth Yamijala added a comment -

          Todd,

          Thanks for the clarification. Yes, I agree that this does seem to be manageable. I am not very sure about the overhead due to the map. So, it would be really nice to see how the memory is impacted with and without the change. Thanks !

          Show
          Hemanth Yamijala added a comment - Todd, Thanks for the clarification. Yes, I agree that this does seem to be manageable. I am not very sure about the overhead due to the map. So, it would be really nice to see how the memory is impacted with and without the change. Thanks !
          Hide
          Todd Lipcon added a comment -

          Hi Hemanth. Since this issue is resolved, I opened a new JIRA to continue the conversation about measuring its memory usage: HADOOP-6488 . (I think it's best practice not to have too much discussion in a resolved ticket as it's harder for people to keep track of. Let me know if you disagree)

          Show
          Todd Lipcon added a comment - Hi Hemanth. Since this issue is resolved, I opened a new JIRA to continue the conversation about measuring its memory usage: HADOOP-6488 . (I think it's best practice not to have too much discussion in a resolved ticket as it's harder for people to keep track of. Let me know if you disagree)
          Hide
          Hemanth Yamijala added a comment -

          I think it's best practice not to have too much discussion in a resolved ticket as it's harder for people to keep track of. Let me know if you disagree

          On the contrary, I cannot agree more heartily smile. Thanks for the new JIRA.

          Show
          Hemanth Yamijala added a comment - I think it's best practice not to have too much discussion in a resolved ticket as it's harder for people to keep track of. Let me know if you disagree On the contrary, I cannot agree more heartily smile . Thanks for the new JIRA.

            People

            • Assignee:
              Todd Lipcon
              Reporter:
              Todd Lipcon
            • Votes:
              0 Vote for this issue
              Watchers:
              8 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development