Chukwa
  1. Chukwa
  2. CHUKWA-267

Collect shuffling information from userlogs/syslog

    Details

    • Type: New Feature New Feature
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.2.0
    • Fix Version/s: 0.1.2, 0.2.0
    • Component/s: Data Collection
    • Labels:
      None
    • Environment:

      Redhat EL 5.1, Java 6

      Description

      Hadoop uses a customized version of log4j appender (org.apache.hadoop.mapred.TaskLogAppender) for tasks. Shuffling information is written to userlogs/syslog by TaskLogAppender. For chukwa to collect shuffling information, we need to write a new TaskLogAppender to do this.

      1. CHUKWA-267-2.patch
        15 kB
        Eric Yang
      2. CHUKWA-267-1.patch
        13 kB
        Eric Yang
      3. CHUKWA-267.patch
        13 kB
        Eric Yang

        Activity

        Hide
        Hudson added a comment -
        Show
        Hudson added a comment - Integrated in Chukwa-trunk #45 (See http://hudson.zones.apache.org/hudson/job/Chukwa-trunk/45/ )
        Hide
        Hudson added a comment -

        Integrated in Chukwa-trunk #44 (See http://hudson.zones.apache.org/hudson/job/Chukwa-trunk/44/)
        . Added TaskLogAppender for collecting Task Tracker Log file. (Eric Yang)

        Show
        Hudson added a comment - Integrated in Chukwa-trunk #44 (See http://hudson.zones.apache.org/hudson/job/Chukwa-trunk/44/ ) . Added TaskLogAppender for collecting Task Tracker Log file. (Eric Yang)
        Eric Yang made changes -
        Status Patch Available [ 10002 ] Resolved [ 5 ]
        Resolution Fixed [ 1 ]
        Hide
        Eric Yang added a comment -

        I just committed this on trunk, thanks Ari, Cheng, and Jerome.

        Show
        Eric Yang added a comment - I just committed this on trunk, thanks Ari, Cheng, and Jerome.
        Hide
        Mac Yang added a comment -

        This is new functionality. I don't think it should go into 0.1.2.

        Show
        Mac Yang added a comment - This is new functionality. I don't think it should go into 0.1.2.
        Hide
        Ari Rabkin added a comment -

        +1 on this, for both 0.1.2 and also Trunk.

        But in the future I think we should probably do the name management on the agent side. This requires changing the agent protocol syntax, so it's a 0.2 feature at least.

        Show
        Ari Rabkin added a comment - +1 on this, for both 0.1.2 and also Trunk. But in the future I think we should probably do the name management on the agent side. This requires changing the agent protocol syntax, so it's a 0.2 feature at least.
        Eric Yang made changes -
        Attachment CHUKWA-267-2.patch [ 12409586 ]
        Hide
        Eric Yang added a comment -

        Update ChukwaDailyRollingFileAppender to use the same class.

        Show
        Eric Yang added a comment - Update ChukwaDailyRollingFileAppender to use the same class.
        Hide
        Jerome Boulon added a comment -

        Eric, this fix this appender but does not fix ChukwaDailyRollingFileAppender.
        So instead of duplicating code and propagate errors or code could you create a singleton class that will be responsible for the registration with our agent.
        This should be a singleton class with only a static access method and a private constructor. That way we can ensure that the code is only at one place and we don;t have to worry about fixing the same code in several classes.

        Also, the same thing could be apply to the way we are writing a log4j event. ChukwaDailyRollingFileAppender is able to write exceptions, I'm not sure if yours is doing that. So could you write an helper class that will be reused cross Chukwa-appender that will do the correct escaping for both standard log4j event and the ones that contain some exception.

        Thanks.

        Show
        Jerome Boulon added a comment - Eric, this fix this appender but does not fix ChukwaDailyRollingFileAppender. So instead of duplicating code and propagate errors or code could you create a singleton class that will be responsible for the registration with our agent. This should be a singleton class with only a static access method and a private constructor. That way we can ensure that the code is only at one place and we don;t have to worry about fixing the same code in several classes. Also, the same thing could be apply to the way we are writing a log4j event. ChukwaDailyRollingFileAppender is able to write exceptions, I'm not sure if yours is doing that. So could you write an helper class that will be reused cross Chukwa-appender that will do the correct escaping for both standard log4j event and the ones that contain some exception. Thanks.
        Eric Yang made changes -
        Attachment CHUKWA-267-1.patch [ 12409331 ]
        Hide
        Eric Yang added a comment -

        Move code block into if(chukwaClient==null) { } to ensure there is only one thread adding the stream to the Chukwa Agent. Removed super.subAppend(); Thanks for the good catches.

        Show
        Eric Yang added a comment - Move code block into if(chukwaClient==null) { } to ensure there is only one thread adding the stream to the Chukwa Agent. Removed super.subAppend(); Thanks for the good catches.
        Hide
        Jerome Boulon added a comment - - edited

        the Add need to be done inside this loop
        if (chukwaClient == null )

        { ... }

        Not sure to understand, why you're writing to a file then you're calling the parent method??
        + this.qw.write(RecordConstants.escapeAllButLastRecordSeparator("\n",this.layout.format(event)));
        + super.subAppend(event);

        Chukwa registration should be extracted to a separate class in order to be reused cross-appender.

        + if (chukwaClientIsNull) {
        + synchronized (chukwaLock) {
        + chukwaClientIsNull = false;
        + String log4jFileName = getFile();
        + String recordType = getRecordType();
        + long currentLength = 0L;
        + if (chukwaClient == null)

        { + chukwaClient = new ChukwaAgentController(); + chukwaClientIsNull = false; + }

        + long adaptorID = chukwaClient.add(ChukwaAgentController.CharFileTailUTF8NewLineEscaped,
        + recordType,currentLength + " " + log4jFileName, currentLength);
        +
        + // Setup a shutdownHook for the controller
        + clientFinalizer = new ClientFinalizer(chukwaClient);
        + Runtime.getRuntime().addShutdownHook(clientFinalizer);
        [...]

        Show
        Jerome Boulon added a comment - - edited the Add need to be done inside this loop if (chukwaClient == null ) { ... } Not sure to understand, why you're writing to a file then you're calling the parent method?? + this.qw.write(RecordConstants.escapeAllButLastRecordSeparator("\n",this.layout.format(event))); + super.subAppend(event); Chukwa registration should be extracted to a separate class in order to be reused cross-appender. + if (chukwaClientIsNull) { + synchronized (chukwaLock) { + chukwaClientIsNull = false; + String log4jFileName = getFile(); + String recordType = getRecordType(); + long currentLength = 0L; + if (chukwaClient == null) { + chukwaClient = new ChukwaAgentController(); + chukwaClientIsNull = false; + } + long adaptorID = chukwaClient.add(ChukwaAgentController.CharFileTailUTF8NewLineEscaped, + recordType,currentLength + " " + log4jFileName, currentLength); + + // Setup a shutdownHook for the controller + clientFinalizer = new ClientFinalizer(chukwaClient); + Runtime.getRuntime().addShutdownHook(clientFinalizer); [...]
        Hide
        Cheng added a comment -

        Suppose two threads reach the point of "synchronized (chukwaLock)" at the same time. After one thread quits from the sync block, another thread will take turn. It means the code inside the sync block will be executed twice. It means the file will be added more than one time; ClientFinalizer will be created more than one time, etc.

        Show
        Cheng added a comment - Suppose two threads reach the point of "synchronized (chukwaLock)" at the same time. After one thread quits from the sync block, another thread will take turn. It means the code inside the sync block will be executed twice. It means the file will be added more than one time; ClientFinalizer will be created more than one time, etc.
        Hide
        Eric Yang added a comment -

        We do not want synchronize to occur on every log statement, the performance will be severely impacted, if we lock on every log statement. Hence, we check for condition first, if there is already one of thread pass through, then rest of the reads don't need to go through the same subroutine. There would be only one thread pass in this subroutine to creates the background thread to register the file. Second, we can't do this in the constructor because we do not know the task attempt id until there is actually something to log, hence this was done at subAppend level.

        Show
        Eric Yang added a comment - We do not want synchronize to occur on every log statement, the performance will be severely impacted, if we lock on every log statement. Hence, we check for condition first, if there is already one of thread pass through, then rest of the reads don't need to go through the same subroutine. There would be only one thread pass in this subroutine to creates the background thread to register the file. Second, we can't do this in the constructor because we do not know the task attempt id until there is actually something to log, hence this was done at subAppend level.
        Hide
        Cheng added a comment - - edited

        Not sure if I understand the code correctly, below are some questions I have.

        1. chukwaClientIsNull and chukwaLock. I guess the correct order might be

        synchronized (chukwaLock) {
        chukwaClientIsNull = false;
        if (chukwaClientIsNull)

        { .............. }

        }

        instead of,

        if (chukwaClientIsNull) {
        synchronized (chukwaLock)

        { chukwaClientIsNull = false; ........... }

        }

        2. Currently in the patch, the chukwaClient is created in method TaskLogAppender.subAppend. If the agent is not running, will the block inside "synchronized (chukwaLock)" called many times? If yes, our appender may impact the task track and then we may need a background thread to create chukwaClient. If no, can we move the code into constructor?

        Thanks,
        Cheng

        -Cheng

        Show
        Cheng added a comment - - edited Not sure if I understand the code correctly, below are some questions I have. 1. chukwaClientIsNull and chukwaLock. I guess the correct order might be synchronized (chukwaLock) { chukwaClientIsNull = false; if (chukwaClientIsNull) { .............. } } instead of, if (chukwaClientIsNull) { synchronized (chukwaLock) { chukwaClientIsNull = false; ........... } } 2. Currently in the patch, the chukwaClient is created in method TaskLogAppender.subAppend. If the agent is not running, will the block inside "synchronized (chukwaLock)" called many times? If yes, our appender may impact the task track and then we may need a background thread to create chukwaClient. If no, can we move the code into constructor? Thanks, Cheng -Cheng
        Eric Yang made changes -
        Status Open [ 1 ] Patch Available [ 10002 ]
        Eric Yang made changes -
        Assignee Eric Yang [ eyang ]
        Eric Yang made changes -
        Field Original Value New Value
        Attachment CHUKWA-267.patch [ 12409311 ]
        Hide
        Eric Yang added a comment -

        Added TaskLogAppender for Chukwa to stream over system log file for task. This will provide extra insight in the task activities.

        Show
        Eric Yang added a comment - Added TaskLogAppender for Chukwa to stream over system log file for task. This will provide extra insight in the task activities.
        Eric Yang created issue -

          People

          • Assignee:
            Eric Yang
            Reporter:
            Eric Yang
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development