Details

    • Type: Bug
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 0.6.0
    • Fix Version/s: 0.8.0
    • Component/s: container
    • Labels:

      Description

      The run-class.sh script defaults to setting up a gc.log file, if SAMZA_OPTS is undefined.

      Newer versions of Java 6, and Java 7 allow you to roll gc log files with -XX:+UseGCLogFileRotation. See http://stackoverflow.com/questions/3822097/rolling-garbage-collector-logs-in-java for details.

      Should also validate that the change doesn't break with older Java 6 versions.

      Also, Suresh Srinivas was saying that there's an alternate way to do this. Suresh - care to chime in?

      1. samza-20.patch
        7 kB
        Raul Castro Fernandez
      2. samza-20-1.patch
        9 kB
        Raul Castro Fernandez

        Issue Links

          Activity

          Hide
          sureshms Suresh Srinivas added a comment -

          Here are gc logs related JVM parameters that we use for Hadoop daemons:

          -Xloggc:/home/gs/var/log/$USER/gc.log-`date +'%Y%m%d%H%M'` -verbose:gc -XX:+PrintGCDetails -XX:+PrintGCTimeStamps -XX:+PrintGCDateStamps
          
          Show
          sureshms Suresh Srinivas added a comment - Here are gc logs related JVM parameters that we use for Hadoop daemons: -Xloggc:/home/gs/var/log/$USER/gc.log-`date +'%Y%m%d%H%M'` -verbose:gc -XX:+PrintGCDetails -XX:+PrintGCTimeStamps -XX:+PrintGCDateStamps
          Hide
          criccomini Chris Riccomini added a comment -

          I don't think injecting the bash date command has quite the same behavior as -XX:+UseGCLogFileRotation. It seems like everyone mis-understood me at the lunch table. What I'm after is GC log rolling of a still-running process, not rolling the logs when the process restarts.

          RFE 6941923 (referenced in the Stack Overflow link) allows you to specify:

          1) -XX:+UseGCLogRotation                       must be used with -Xloggc:<filename>
          2) -XX:NumberOfGClogFiles=<number of files>    must be >=1, default is one
          3) -XX:GCLogFileSize=<number>M (or K)          default will be set to 512K
          
          Show
          criccomini Chris Riccomini added a comment - I don't think injecting the bash date command has quite the same behavior as -XX:+UseGCLogFileRotation. It seems like everyone mis-understood me at the lunch table. What I'm after is GC log rolling of a still-running process, not rolling the logs when the process restarts. RFE 6941923 (referenced in the Stack Overflow link) allows you to specify: 1) -XX:+UseGCLogRotation must be used with -Xloggc:<filename> 2) -XX:NumberOfGClogFiles=<number of files> must be >=1, default is one 3) -XX:GCLogFileSize=<number>M (or K) default will be set to 512K
          Hide
          farisa Adam Faris added a comment -

          Chris, I think this might help with what you're asking:
          http://logging.apache.org/log4j/1.2/apidocs/org/apache/log4j/DailyRollingFileAppender.html

          We use it in our hive log4j configs and have something like this.

          hive.root.logger=WARN,DRFA
          hive.log.dir=/my/log/path/${user.name}
          hive.log.file=hive.log
          
          # Define the root logger to the system property "hadoop.root.logger".
          log4j.rootLogger=${hive.root.logger}, EventCounter
          
          # Logging Threshold
          log4j.threshhold=WARN
          
          #
          # Daily Rolling File Appender
          #
          
          log4j.appender.DRFA=org.apache.log4j.DailyRollingFileAppender
          log4j.appender.DRFA.File=${hive.log.dir}/${hive.log.file}
          
          # Rollver at midnight
          log4j.appender.DRFA.DatePattern=.yyyy-MM-dd
          
          # 30-day backup
          #log4j.appender.DRFA.MaxBackupIndex=30
          log4j.appender.DRFA.layout=org.apache.log4j.PatternLayout
          
          # Pattern format: Date LogLevel LoggerName LogMessage
          #log4j.appender.DRFA.layout.ConversionPattern=%d{ISO8601} %p %c: %m%n
          # Debugging Pattern format
          log4j.appender.DRFA.layout.ConversionPattern=%d{ISO8601} %-5p %c{2} (%F:%M(%L)) - %m%n
          
          Show
          farisa Adam Faris added a comment - Chris, I think this might help with what you're asking: http://logging.apache.org/log4j/1.2/apidocs/org/apache/log4j/DailyRollingFileAppender.html We use it in our hive log4j configs and have something like this. hive.root.logger=WARN,DRFA hive.log.dir=/my/log/path/${user.name} hive.log.file=hive.log # Define the root logger to the system property "hadoop.root.logger" . log4j.rootLogger=${hive.root.logger}, EventCounter # Logging Threshold log4j.threshhold=WARN # # Daily Rolling File Appender # log4j.appender.DRFA=org.apache.log4j.DailyRollingFileAppender log4j.appender.DRFA.File=${hive.log.dir}/${hive.log.file} # Rollver at midnight log4j.appender.DRFA.DatePattern=.yyyy-MM-dd # 30-day backup #log4j.appender.DRFA.MaxBackupIndex=30 log4j.appender.DRFA.layout=org.apache.log4j.PatternLayout # Pattern format: Date LogLevel LoggerName LogMessage #log4j.appender.DRFA.layout.ConversionPattern=%d{ISO8601} %p %c: %m%n # Debugging Pattern format log4j.appender.DRFA.layout.ConversionPattern=%d{ISO8601} %-5p %c{2} (%F:%M(%L)) - %m%n
          Hide
          raulcf Raul Castro Fernandez added a comment -

          Just added a patch. Link to the review board:
          https://reviews.apache.org/r/22421/

          Show
          raulcf Raul Castro Fernandez added a comment - Just added a patch. Link to the review board: https://reviews.apache.org/r/22421/
          Hide
          martinkl Martin Kleppmann added a comment -

          The parsing of the Java version number seems quite fragile. Not sure if non-Hotspot JVMs will match the output of $(java -version) closely enough for this to work.

          Do you have access to an older/non-Hotspot JVM to test it? It would be nicest if we could simply always set the option (if we are confident that older JVMs will ignore it, not fail).

          Also, have you tested whether the GC rolling actually works? In your patch it looks like you're missing a space between two of the options.

          Show
          martinkl Martin Kleppmann added a comment - The parsing of the Java version number seems quite fragile. Not sure if non-Hotspot JVMs will match the output of $(java -version) closely enough for this to work. Do you have access to an older/non-Hotspot JVM to test it? It would be nicest if we could simply always set the option (if we are confident that older JVMs will ignore it, not fail). Also, have you tested whether the GC rolling actually works? In your patch it looks like you're missing a space between two of the options.
          Hide
          raulcf Raul Castro Fernandez added a comment -

          It would be nicest if we could simply always set the option (if we are confident that older JVMs will ignore it, not fail).

          Yes. However, if the option is enabled and not supported by the JVM, there is an error that prevents creating the JVM (see below).

          Unrecognized VM option '+UseGCLogFileRotation'
          Could not create the Java virtual machine.

          Also, have you tested whether the GC rolling actually works? In your patch it looks like you're missing a space between two of the options.

          Yes, I'm actually going to explicitly write the other options for GC fileRotate (num of files and size) as this seems to be the best way to configure this parameter anyway.

          The parsing of the Java version number seems quite fragile. Not sure if non-Hotspot JVMs will match the output of $(java -version) closely enough for this to work.

          Agree. I don't have access to non-Hotspot JVMs tough. I'll see if I can try in one and check whether the output of version is the same or not.

          Show
          raulcf Raul Castro Fernandez added a comment - It would be nicest if we could simply always set the option (if we are confident that older JVMs will ignore it, not fail). Yes. However, if the option is enabled and not supported by the JVM, there is an error that prevents creating the JVM (see below). Unrecognized VM option '+UseGCLogFileRotation' Could not create the Java virtual machine. Also, have you tested whether the GC rolling actually works? In your patch it looks like you're missing a space between two of the options. Yes, I'm actually going to explicitly write the other options for GC fileRotate (num of files and size) as this seems to be the best way to configure this parameter anyway. The parsing of the Java version number seems quite fragile. Not sure if non-Hotspot JVMs will match the output of $(java -version) closely enough for this to work. Agree. I don't have access to non-Hotspot JVMs tough. I'll see if I can try in one and check whether the output of version is the same or not.
          Hide
          criccomini Chris Riccomini added a comment -

          Yes. However, if the option is enabled and not supported by the JVM, there is an error that prevents creating the JVM (see below). Unrecognized VM option '+UseGCLogFileRotation' Could not create the Java virtual machine.

          Rather than the version parsing logic, could we simply start a java process with the UseGCLogFileRotation set, and check the exit code? Something like:

          java -XX:+UseGCLogFileRotation -version
          
          Show
          criccomini Chris Riccomini added a comment - Yes. However, if the option is enabled and not supported by the JVM, there is an error that prevents creating the JVM (see below). Unrecognized VM option '+UseGCLogFileRotation' Could not create the Java virtual machine. Rather than the version parsing logic, could we simply start a java process with the UseGCLogFileRotation set, and check the exit code? Something like: java -XX:+UseGCLogFileRotation -version
          Hide
          criccomini Chris Riccomini added a comment -

          This seems to work with my JVM:

          java -XX:+UseGCLogFileRotation -Xloggc:/tmp/foo -XX:NumberOfGCLogFiles=10 -XX:GCLogFileSize=10241024 -version
          

          We should validate on an older JVM.

          Show
          criccomini Chris Riccomini added a comment - This seems to work with my JVM: java -XX:+UseGCLogFileRotation -Xloggc:/tmp/foo -XX:NumberOfGCLogFiles=10 -XX:GCLogFileSize=10241024 -version We should validate on an older JVM.
          Hide
          raulcf Raul Castro Fernandez added a comment -

          could we simply start a java process with the UseGCLogFileRotation set, and check the exit code?

          Ok. The new patch is now doing this

          We should validate on an older JVM.

          I have tested this with 2 different HotSpot JVMs (one that supports the option and one that does not).

          Show
          raulcf Raul Castro Fernandez added a comment - could we simply start a java process with the UseGCLogFileRotation set, and check the exit code? Ok. The new patch is now doing this We should validate on an older JVM. I have tested this with 2 different HotSpot JVMs (one that supports the option and one that does not).
          Hide
          criccomini Chris Riccomini added a comment -

          Cool. Only other nit is rather than duplicating this chunk of parameters twice:

          JAVA_OPTS="-Xmx768M -XX:+PrintGCDateStamps -Xloggc:$SAMZA_LOG_DIR/gc.log -Dsamza.log.dir=$SAMZA_LOG_DIR -Dsamza.container.name=$SAMZA_CONTAINER_NAME"
          

          Can we just set it once, and then have the if/fi statement append the file rotation stuff? Seems a little cleaner.

          Show
          criccomini Chris Riccomini added a comment - Cool. Only other nit is rather than duplicating this chunk of parameters twice: JAVA_OPTS="-Xmx768M -XX:+PrintGCDateStamps -Xloggc:$SAMZA_LOG_DIR/gc.log -Dsamza.log.dir=$SAMZA_LOG_DIR -Dsamza.container.name=$SAMZA_CONTAINER_NAME" Can we just set it once, and then have the if/fi statement append the file rotation stuff? Seems a little cleaner.
          Hide
          raulcf Raul Castro Fernandez added a comment -

          Ok. Just grouped those parameters into two variables, one for the common parameters, and the other for the log file retention ones. I had to decouple the Xloggc tough, as this receives a different path depending on if we are "trying" the option or setting it.

          Show
          raulcf Raul Castro Fernandez added a comment - Ok. Just grouped those parameters into two variables, one for the common parameters, and the other for the log file retention ones. I had to decouple the Xloggc tough, as this receives a different path depending on if we are "trying" the option or setting it.
          Hide
          criccomini Chris Riccomini added a comment -

          I think we don't need the GC_LOG_OUTPUT variable. We had -Xloggc:$SAMZA_LOG_DIR/gc.log in JAVA_OPTS before, and I think we need it regardless of whether we roll or not. COMMON_PARAMS seems like the right spot for it.

          Show
          criccomini Chris Riccomini added a comment - I think we don't need the GC_LOG_OUTPUT variable. We had -Xloggc:$SAMZA_LOG_DIR/gc.log in JAVA_OPTS before, and I think we need it regardless of whether we roll or not. COMMON_PARAMS seems like the right spot for it.
          Hide
          criccomini Chris Riccomini added a comment -

          I had to decouple the Xloggc tough, as this receives a different path depending on if we are "trying" the option or setting it.

          Ah, I see. I think it's still safe to put it in COMMON_PARAMS, though. You only use GC_LOG_ROTATION_PARAMS in the test, so you can hard-code the Xloggc there, and leave the regular setting in COMMON_PARAMS.

          Show
          criccomini Chris Riccomini added a comment - I had to decouple the Xloggc tough, as this receives a different path depending on if we are "trying" the option or setting it. Ah, I see. I think it's still safe to put it in COMMON_PARAMS, though. You only use GC_LOG_ROTATION_PARAMS in the test, so you can hard-code the Xloggc there, and leave the regular setting in COMMON_PARAMS.
          Hide
          raulcf Raul Castro Fernandez added a comment -

          I think it's still safe to put it in COMMON_PARAMS, though. You only use GC_LOG_ROTATION_PARAMS in the test, so you can hard-code the Xloggc there, and leave the regular setting in COMMON_PARAMS

          Ah, true!
          I did that and renamed PARAMS to OPTS to be consistent with JAVA_OPTS

          Show
          raulcf Raul Castro Fernandez added a comment - I think it's still safe to put it in COMMON_PARAMS, though. You only use GC_LOG_ROTATION_PARAMS in the test, so you can hard-code the Xloggc there, and leave the regular setting in COMMON_PARAMS Ah, true! I did that and renamed PARAMS to OPTS to be consistent with JAVA_OPTS
          Hide
          criccomini Chris Riccomini added a comment -

          +1 Merged and committed.

          Show
          criccomini Chris Riccomini added a comment - +1 Merged and committed.

            People

            • Assignee:
              raulcf Raul Castro Fernandez
              Reporter:
              criccomini Chris Riccomini
            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development