<html>
  <head>
    <meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
  </head>
  <body text="#000000" bgcolor="#FFFFFF">
    Tao,<br>
    <br>
      Thanks for your review.<br>
    <br>
    <div class="moz-cite-prefix">On 8/23/2013 3:33 PM, Tao Mao wrote:<br>
    </div>
    <blockquote cite="mid:5217E333.9020802@oracle.com" type="cite">
      <meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
      Hi,<br>
      <br>
      I reviewed most of the code and test-ran a build from it. It's a
      very cool and important improvement. <br>
      <br>
      Thank you for putting together these on our wishlist for long.<br>
      <br>
      Below are some comments.<br>
      <br>
      1. src/share/vm/runtime/arguments.cpp<br>
      <br>
      - 1853 "To enable GC log rotation, use -Xloggc:&lt;filename&gt;
      -XX:+UseGCLogFileRotation
      -XX:NumberOfGCLogFiles=&lt;num_of_files&gt;
      -XX:GCLogFileSize=&lt;num_of_size&gt;[k|K|m|M]\n"<br>
      + 1853 "To enable GC log rotation, use -Xloggc:&lt;filename&gt;
      -XX:+UseGCLogFileRotation
      -XX:NumberOfGCLogFiles=&lt;num_of_files&gt;
      -XX:GCLogFileSize=&lt;num_of_size&gt;[k|K|m|M|g|G]\n"<br>
      <br>
      Please consider adding [g|G] to GCLogFileSize suggestion.<br>
      <br>
      I worked on a problem of enabling gc log limit over 2G
      (JDK-7122222). So it seems that customers sometimes want gc logs
      to be very large.<br>
      <br>
    </blockquote>
    Sure, will add.<br>
    <blockquote cite="mid:5217E333.9020802@oracle.com" type="cite"> 2.
      src/share/vm/runtime/arguments.hpp<br>
      <br>
      (1) with the current changeset, we only append date&amp;time to
      file_name w/ +UseGCLogFileRotation; if not, we won't have
      date&amp;time info. <br>
      <br>
      I think it would be useful to let both cases (w/ and w/o
      UseGCLogFileRotation) have date&amp;time in order to solve the
      overwritten problem (e.g. JDK-8020667). In fact, having that, we
      actually solve JDK-8020667.<br>
      <br>
      If you want to have that, basically you need to work on the
      FileStream constructor methods fileStream().<br>
      <br>
    </blockquote>
    I can change that, if no objection from others. This also will
    simplify the setting of file name here.<br>
    <br>
    <blockquote cite="mid:5217E333.9020802@oracle.com" type="cite"> (2)
      Would it be better to rename method name reformat_filename() to
      extend_filename()?<br>
      <br>
      (3) Typos and suggestion<br>
      537 // rotate file in names filename.0, filename.1, ...,
      filename.&lt;NumberOfGCLogFiles - 1&gt; <br>
      <b>=&gt; extended_filename.0</b><br>
      <br>
      538 // filename contains pid and time when the fist file created.
      The current filename is <br>
      <b>=&gt; </b>the<b> first </b>file created.<br>
      <br>
      539 // gc_log_file_name + pid&lt;pid&gt; +
      YYYY-MM-DD_HH-MM-SS.&lt;i&gt;.current, where i is current <br>
      540 // rotation file number. After it reaches max file size, the
      file will be saved and <br>
      541 // renamed with .current removed from its tail.<br>
      <br>
    </blockquote>
    Will change that.<br>
    <blockquote cite="mid:5217E333.9020802@oracle.com" type="cite"> 3.
      For your point 5), I don't quite get it. In my test-run, I tried
      to change file permission to unreadable and unwritable, but VM
      would later change the permission back anyway.<br>
      <br>
      So could you bring up some use cases of that functionality to give
      more details?<br>
      <br>
    </blockquote>
    Changing file permission will not stop the file create, in this
    rotation, the file opened/saved/removed/renamed -&gt; then repeat.
    What I have done is change the while directory to read only, then
    the create failed so rotation stopped.<br>
    <br>
    <blockquote cite="mid:5217E333.9020802@oracle.com" type="cite"> 4.
      Will you write jtreg tests for this functionality? It looks
      possible to write some tests, at least checking the format of log
      names.<br>
      <br>
    </blockquote>
    Good suggestion, I will add one.<br>
    <br>
    <blockquote cite="mid:5217E333.9020802@oracle.com" type="cite">
      <meta charset="utf-8">
      Thanks.<br>
      Tao<br>
      <br>
      On 8/23/13 7:53 AM, Yumin Qi wrote:
      <blockquote cite="mid:52177780.3040109@oracle.com" type="cite">
        <meta content="text/html; charset=UTF-8"
          http-equiv="Content-Type">
        Could I get  a GC staff review the change? Need more reviewers
        to push this in.<br>
        <br>
        Thanks<br>
        Yumin<br>
        <br>
        <div class="moz-cite-prefix">On 8/21/2013 3:43 PM, Yumin Qi
          wrote:<br>
        </div>
        <blockquote cite="mid:521542A3.3020906@oracle.com" type="cite">
          <meta content="text/html; charset=UTF-8"
            http-equiv="Content-Type">
          Hi, all<br>
          <br>
            This is second version after feedback from first round.<br>
            The changes are:<br>
          <br>
            1) file name will be based on gc log file name
          (-Xloggc:filename), pid (process id) and time when the first
          rotation file created:<br>
                 &lt;filename&gt;-pid&lt;pid&gt;-YYYY-MM-DD_HH-MM-SS<br>
            2) If rotate in same file, file name is as in 1), if rotate
          in multiple files, .&lt;i&gt; will append to above file name.<br>
            3) every time file rotated, file name and time when file
          created will be logged to head/tail, this is same as first
          version.<br>
            4) current file has name 
          &lt;filename&gt;-pid&lt;pid&gt;-YYYY-MM-DD_HH-MM-SS.&lt;i&gt;.current<br>
                 This is similar to first version.<br>
                 By adapting such name format we will never loss logs in
          case apps stops and restart, the log files will not be
          overwritten since time stamp in file names.<br>
             5) If open file failed, turn off gc log rotation. <br>
                  If due to some reason, file operation failed (such as
          permission changed etc), with log file opened, logging still
          works, but at <br>
                  saving and renaming, the file operation will fail,
          this will lead not all files saved.<br>
          <br>
               <a moz-do-not-send="true" class="moz-txt-link-freetext"
            href="http://cr.openjdk.java.net/%7Eminqi/7164841/webrev01">http://cr.openjdk.java.net/~minqi/7164841/webrev01</a><br>
               <br>
               Tested with jtreg, JPRT.<br>
          <br>
          Thanks<br>
          Yumin<br>
          <br>
          <div class="moz-cite-prefix">On 8/15/2013 8:35 AM, Yumin Qi
            wrote:<br>
          </div>
          <blockquote cite="mid:520CF52B.6050000@oracle.com" type="cite">
            <meta http-equiv="content-type" content="text/html;
              charset=UTF-8">
            Hi, <br>
            <br>
              Can I have your review for this small changes?<br>
              <a moz-do-not-send="true"
              href="http://cr.openjdk.java.net/%7Eminqi/7164841/webrev00/">http://cr.openjdk.java.net/~minqi/7164841/webrev00/</a><br>
            <br>
               This is for a enhancement to add head/tail message to the
            logging files to assist reading GC output.<br>
               1. modified prompt message if invalid arguments used for
            log rotating;<br>
               2. add time and file name message to log file head/tail.<br>
               3. for easily identify which log file is current, use
            file name like &lt;filename&gt;.n.current, after it reaches
            maximum size, rename it to &lt;filename&gt;.n<br>
                    On Windows, there is no F_OK (existing test)
            definition, F_OK is defined as "0" and for _access of VC++,
            it just describes:<br>
                     <br>
            <table style="border-collapse: collapse; padding: 0px;
              width: 1306px; border: 1px solid rgb(187, 187, 187);
              color: rgb(0, 0, 0); font-family: 'Segoe UI', 'Lucida
              Grande', Verdana, Arial, Helvetica, sans-serif; font-size:
              13px; font-style: normal; font-variant: normal;
              font-weight: normal; letter-spacing: normal; line-height:
              17px; orphans: auto; text-align: start; text-indent: 0px;
              text-transform: none; white-space: normal; widows: auto;
              word-spacing: 0px; -webkit-text-stroke-width: 0px;">
              <tbody>
                <tr>
                  <th style="border: 1px solid rgb(187, 187, 187);
                    margin: 10px; padding: 10px 8px; background-color:
                    rgb(237, 237, 237); color: rgb(112, 112, 112);
                    text-align: left;">
                    <p style="color: rgb(42, 42, 42); margin-top: 0px;
                      margin-bottom: 0px; padding-bottom: 0px;
                      line-height: 18px;"><span class="parameter"
                        style="font-style: italic;">mode</span><span
                        class="Apple-converted-space"> </span>value</p>
                  </th>
                  <th style="border: 1px solid rgb(187, 187, 187);
                    margin: 10px; padding: 10px 8px; background-color:
                    rgb(237, 237, 237); color: rgb(112, 112, 112);
                    text-align: left;">
                    <p style="color: rgb(42, 42, 42); margin-top: 0px;
                      margin-bottom: 0px; padding-bottom: 0px;
                      line-height: 18px;">Checks file for</p>
                  </th>
                </tr>
                <tr>
                  <td style="border: 1px solid rgb(187, 187, 187);
                    margin: 10px; padding: 10px 8px; color: rgb(42, 42,
                    42); vertical-align: top;">
                    <p style="color: rgb(42, 42, 42); margin-top: 0px;
                      margin-bottom: 0px; padding-bottom: 0px;
                      line-height: 18px;">00</p>
                  </td>
                  <td style="border: 1px solid rgb(187, 187, 187);
                    margin: 10px; padding: 10px 8px; color: rgb(42, 42,
                    42); vertical-align: top;">
                    <p style="color: rgb(42, 42, 42); margin-top: 0px;
                      margin-bottom: 0px; padding-bottom: 0px;
                      line-height: 18px;">Existence only</p>
                  </td>
                </tr>
                <tr>
                  <td style="border: 1px solid rgb(187, 187, 187);
                    margin: 10px; padding: 10px 8px; color: rgb(42, 42,
                    42); vertical-align: top;">
                    <p style="color: rgb(42, 42, 42); margin-top: 0px;
                      margin-bottom: 0px; padding-bottom: 0px;
                      line-height: 18px;">02</p>
                  </td>
                  <td style="border: 1px solid rgb(187, 187, 187);
                    margin: 10px; padding: 10px 8px; color: rgb(42, 42,
                    42); vertical-align: top;">
                    <p style="color: rgb(42, 42, 42); margin-top: 0px;
                      margin-bottom: 0px; padding-bottom: 0px;
                      line-height: 18px;">Write-only</p>
                  </td>
                </tr>
                <tr>
                  <td style="border: 1px solid rgb(187, 187, 187);
                    margin: 10px; padding: 10px 8px; color: rgb(42, 42,
                    42); vertical-align: top;">
                    <p style="color: rgb(42, 42, 42); margin-top: 0px;
                      margin-bottom: 0px; padding-bottom: 0px;
                      line-height: 18px;">04</p>
                  </td>
                  <td style="border: 1px solid rgb(187, 187, 187);
                    margin: 10px; padding: 10px 8px; color: rgb(42, 42,
                    42); vertical-align: top;">
                    <p style="color: rgb(42, 42, 42); margin-top: 0px;
                      margin-bottom: 0px; padding-bottom: 0px;
                      line-height: 18px;">Read-only</p>
                  </td>
                </tr>
                <tr>
                  <td style="border: 1px solid rgb(187, 187, 187);
                    margin: 10px; padding: 10px 8px; color: rgb(42, 42,
                    42); vertical-align: top;">
                    <p style="color: rgb(42, 42, 42); margin-top: 0px;
                      margin-bottom: 0px; padding-bottom: 0px;
                      line-height: 18px;">06</p>
                  </td>
                  <td style="border: 1px solid rgb(187, 187, 187);
                    margin: 10px; padding: 10px 8px; color: rgb(42, 42,
                    42); vertical-align: top;">
                    <p style="color: rgb(42, 42, 42); margin-top: 0px;
                      margin-bottom: 0px; padding-bottom: 0px;
                      line-height: 18px;">Read and write</p>
                  </td>
                </tr>
              </tbody>
            </table>
            <br>
            <a moz-do-not-send="true"
              href="http://msdn.microsoft.com/en-us/library/1w06ktdy.aspx">http://msdn.microsoft.com/en-us/library/1w06ktdy.aspx</a><br>
            The definition are consistent with unistd.h.       <br>
            <br>
                Test: JPRT and jtreg.<br>
            <br>
               Thanks<br>
               Yumin<br>
          </blockquote>
          <br>
        </blockquote>
        <br>
      </blockquote>
    </blockquote>
    <br>
  </body>
</html>