Uploaded image for project: 'Chukwa'
  1. Chukwa
  2. CHUKWA-28

Add late initialization to the chukwa log4j appender

    Details

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

      Description

      With the current Chukwa log4j implementation, if you define a static log4j configuration the appender is initialized at creation time.

      This may cause some problems for example with Hadoop Audit log or Metrics context, if the log4j properties are statically defined then a userX running an hadoop command will trigger a permission denied exception since the appender will try to initialize itself and therefore try to create a file under userX ownership for each appender even if not data is going to be written to it.

      The goal is to delay this initialization until the first message is actually written to that log so this kind of issue could easily be avoided.

      1. CHUKWA-28.patch
        7 kB
        Jerome Boulon
      2. HADOOP-5058.patch
        7 kB
        Jerome Boulon
      3. HADOOP-5058-2.patch
        7 kB
        Jerome Boulon

        Issue Links

          Activity

          Hide
          jboulon Jerome Boulon added a comment -
          • Move clientFinalizer definition to the top
          • Delayed file initialization/creation until first log event
          • do a chmod 640 on the file in order to give read access to Chukwa.
            This should be revisit when we will start working on security.
          Show
          jboulon Jerome Boulon added a comment - Move clientFinalizer definition to the top Delayed file initialization/creation until first log event do a chmod 640 on the file in order to give read access to Chukwa. This should be revisit when we will start working on security.
          Hide
          hadoopqa Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12401153/HADOOP-5058.patch
          against trunk revision 748861.

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

          +1 tests included. The patch appears to include 4 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 Eclipse classpath. The patch retains Eclipse classpath integrity.

          -1 release audit. The applied patch generated 799 release audit warnings (more than the trunk's current 798 warnings).

          -1 core tests. The patch failed core unit tests.

          -1 contrib tests. The patch failed contrib unit tests.

          Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/29/testReport/
          Release audit warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/29/artifact/trunk/current/releaseAuditDiffWarnings.txt
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/29/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/29/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/29/console

          This message is automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12401153/HADOOP-5058.patch against trunk revision 748861. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 4 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 Eclipse classpath. The patch retains Eclipse classpath integrity. -1 release audit. The applied patch generated 799 release audit warnings (more than the trunk's current 798 warnings). -1 core tests. The patch failed core unit tests. -1 contrib tests. The patch failed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/29/testReport/ Release audit warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/29/artifact/trunk/current/releaseAuditDiffWarnings.txt Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/29/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/29/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/29/console This message is automatically generated.
          Hide
          jboulon Jerome Boulon added a comment -

          .TestChukwaDailyRollingFileAppender.testlateInit4ChukwaDailyRollingFileAppender failed because I forgot to commit the latest version of build.xml

          org.apache.hadoop.chukwa.datacollection.adaptor.filetailer.TestStartAtOffset.testStartAtOffset is failing because of a bug not related to this patch, fileName is "0 /tmp/chukwaTest" instead of "/tmp/chukwaTest". My guess is that this bug has been introduced by HADOOP-5087
          (The regex in FileTailingAdaptor is no longer valid with HADOOP-5087)

          org.apache.hadoop.mapred.TestJobSysDirWithDFS.testWithDFS & org.apache.hadoop.mapred.TestJobHistory.testJobHistoryUserLogLocation are not related to that patch

          Show
          jboulon Jerome Boulon added a comment - .TestChukwaDailyRollingFileAppender.testlateInit4ChukwaDailyRollingFileAppender failed because I forgot to commit the latest version of build.xml org.apache.hadoop.chukwa.datacollection.adaptor.filetailer.TestStartAtOffset.testStartAtOffset is failing because of a bug not related to this patch, fileName is "0 /tmp/chukwaTest" instead of "/tmp/chukwaTest". My guess is that this bug has been introduced by HADOOP-5087 (The regex in FileTailingAdaptor is no longer valid with HADOOP-5087 ) org.apache.hadoop.mapred.TestJobSysDirWithDFS.testWithDFS & org.apache.hadoop.mapred.TestJobHistory.testJobHistoryUserLogLocation are not related to that patch
          Hide
          asrabkin Ari Rabkin added a comment -

          I don't understand why we need the chmod – what are the files created as, by default?

          Show
          asrabkin Ari Rabkin added a comment - I don't understand why we need the chmod – what are the files created as, by default?
          Hide
          jboulon Jerome Boulon added a comment -

          It depends on the user umask and since chukwaAgent is not running as a privilege user, most of the time you'll not have access to it.
          BTW, this has been there for a while in Log4JMetricsContext.java

          Show
          jboulon Jerome Boulon added a comment - It depends on the user umask and since chukwaAgent is not running as a privilege user, most of the time you'll not have access to it. BTW, this has been there for a while in Log4JMetricsContext.java
          Hide
          asrabkin Ari Rabkin added a comment -

          Let's give it a whirl! +1

          Show
          asrabkin Ari Rabkin added a comment - Let's give it a whirl! +1
          Hide
          jboulon Jerome Boulon added a comment -

          copy late-log4j.properties to the right location

          Show
          jboulon Jerome Boulon added a comment - copy late-log4j.properties to the right location
          Hide
          eyang Eric Yang added a comment -

          Resubmit for hudson test.

          Show
          eyang Eric Yang added a comment - Resubmit for hudson test.
          Hide
          eyang Eric Yang added a comment -

          Comment from previous release note:

          A junit is included but if you want to see the default behavior:
          Edit: src/contrib/chukwa/src/test/org/apache/hadoop/chukwa/inputtools/log4j/late-log4j.properties
          and change it according to:
          log4j.appender.R=org.apache.log4j.RollingFileAppender
          #log4j.appender.R=org.apache.hadoop.chukwa.inputtools.log4j.ChukwaDailyRollingFileAppender

          Show
          eyang Eric Yang added a comment - Comment from previous release note: A junit is included but if you want to see the default behavior: Edit: src/contrib/chukwa/src/test/org/apache/hadoop/chukwa/inputtools/log4j/late-log4j.properties and change it according to: log4j.appender.R=org.apache.log4j.RollingFileAppender #log4j.appender.R=org.apache.hadoop.chukwa.inputtools.log4j.ChukwaDailyRollingFileAppender
          Hide
          asrabkin Ari Rabkin added a comment -

          I'd like to commit this, but the patch has gone stale. Jerome, can you regenerate?

          Show
          asrabkin Ari Rabkin added a comment - I'd like to commit this, but the patch has gone stale. Jerome, can you regenerate?
          Hide
          jboulon Jerome Boulon added a comment -

          Same patch based on CHUKWA repo

          Show
          jboulon Jerome Boulon added a comment - Same patch based on CHUKWA repo
          Hide
          jboulon Jerome Boulon added a comment -

          Same patch but based on CHUKWA tree

          Show
          jboulon Jerome Boulon added a comment - Same patch but based on CHUKWA tree
          Hide
          asrabkin Ari Rabkin added a comment -

          I just committed this. thanks, Jerome!

          Note that this patch also alters the display of test results. This was discussed by myself, Jerome, and Eric earlier today, and I'm happy to commit it.

          Also. Should there be a release note for this?

          Show
          asrabkin Ari Rabkin added a comment - I just committed this. thanks, Jerome! Note that this patch also alters the display of test results. This was discussed by myself, Jerome, and Eric earlier today, and I'm happy to commit it. Also. Should there be a release note for this?

            People

            • Assignee:
              jboulon Jerome Boulon
              Reporter:
              jboulon Jerome Boulon
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development