[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: RFR: 8146879: Add option for handling existing log files in UL
From: Marcus Larsson <marcus.larsson () oracle ! com>
Date: 2016-03-31 7:26:02
Message-ID: 56FCD10A.6050200 () oracle ! com
[Download RAW message or body]
Thanks for reviewing!
Marcus
On 03/30/2016 06:16 PM, Gerard Ziemski wrote:
> Look good.
>
> Thank you for you patience with my review.
>
>
> cheers
>
> > On Mar 30, 2016, at 9:17 AM, Marcus Larsson <marcus.larsson@oracle.com> wrote:
> >
> > Hi Gerard,
> >
> > On 03/22/2016 07:15 PM, Gerard Ziemski wrote:
> > > hi Marcus,
> > >
> > > Thank you for incorporating the feedback.
> > >
> > > The only remaining comment I have is about the \
> > > "number_of_lines_with_substring_in_file()" function. As written, it resets the \
> > > entire algorithm to the beginning of the file when it can not fit the current \
> > > line, when I think the intention should be only to rewind only to the beginning \
> > > of that line.
> > > May I suggest the following implementation instead:
> > >
> > > static size_t number_of_lines_with_substring_in_file(const char* filename,
> > > const char* substr) {
> > > ResourceMark rm;
> > > size_t ret = 0;
> > > FILE* fp = fopen(filename, "r");
> > > assert(fp, "error opening file %s: %s", filename, strerror(errno));
> > >
> > > int buflen = 512;
> > > char* buf = NEW_RESOURCE_ARRAY(char, buflen);
> > > long int pos = 0;
> > >
> > > while (fgets(buf, buflen, fp) != NULL) {
> > > if (buf[strlen(buf) - 1] != '\n') {
> > > // rewind to the beginning of the line
> > > fseek(fp, pos, SEEK_SET);
> > > // double the buffer size and try again
> > > buf = REALLOC_RESOURCE_ARRAY(char, buf, buflen, 2*buflen);
> > > buflen *= 2;
> > > } else {
> > > // get current file position
> > > pos = ftell(fp);
> > >
> > > if (strstr(buf, substr) != NULL) {
> > > ret++;
> > > }
> > > }
> > > }
> > >
> > > fclose(fp);
> > > return ret;
> > > }
> > Alright, fixed.
> >
> > New webrev:
> > http://cr.openjdk.java.net/~mlarsson/8146879/webrev.05/
> >
> > Incremental:
> > http://cr.openjdk.java.net/~mlarsson/8146879/webrev.04-05/
> >
> > Also fixed some failing tests caused by this patch in the previous round of \
> > changes.
> > Thanks,
> > Marcus
> >
> > > cheers
> > >
> > >
> > > > On Mar 18, 2016, at 8:04 AM, Marcus Larsson <marcus.larsson@oracle.com> \
> > > > wrote:
> > > > Updated patch after feedback.
> > > >
> > > > New webrev:
> > > > http://cr.openjdk.java.net/~mlarsson/8146879/webrev.04/
> > > >
> > > > Incremental:
> > > > http://cr.openjdk.java.net/~mlarsson/8146879/webrev.03-04/
> > > >
> > > > Tested with internal VM tests through RBT.
> > > >
> > > > Changes:
> > > > * Rotation filecount is now limited to 1000 files.
> > > > * Removed loop in os::compare_file_modified_times.
> > > > * Added a check before rotating/truncating an existing log file, and will \
> > > > only do so if it's a regular file.
> > > > * Added test case to check that logging to a directory gives the intended \
> > > > error message.
> > > > * Fixed test help method to handle arbitrary length log lines.
> > > >
> > > > Thanks,
> > > > Marcus
> > > >
> > > > On 03/11/2016 03:21 PM, Marcus Larsson wrote:
> > > > > Third time's the charm.
> > > > >
> > > > > Webrev:
> > > > > http://cr.openjdk.java.net/~mlarsson/8146879/webrev.03/
> > > > >
> > > > > This patch makes log file rotation the default. Default thresholds are 5 \
> > > > > rotated files with a target size of 20MiB. Truncating behavior can be \
> > > > > achieved by setting filecount to 0 (-Xlog::myfile.log::filecount=0).
> > > > > If a log file already exists during log file initialization it will be \
> > > > > rotated. If any of the target file names (file.0 to file.4 in the default \
> > > > > case) are available, that filename will be used for the existing log. If \
> > > > > all names are taken the VM will attempt to overwrite the oldest file.
> > > > > This should prevent unlimited log file creations and avoid accidental loss \
> > > > > of log files from previous runs. The default thresholds (5 files, 20MiB \
> > > > > each) is just a suggestion. If you think it should be higher/lower let me \
> > > > > know.
> > > > > Tested with included internal VM tests through RBT.
> > > > >
> > > > > Thanks,
> > > > > Marcus
> > > > >
> > > > > On 2016-03-01 15:05, Marcus Larsson wrote:
> > > > > > Hi,
> > > > > >
> > > > > > After some offline discussions I'm withdrawing this patch. I will instead \
> > > > > > investigate if I can achieve similar behavior using log rotation as the \
> > > > > > default.
> > > > > > Thanks,
> > > > > > Marcus
> > > > > >
> > > > > > On 03/01/2016 12:11 PM, Marcus Larsson wrote:
> > > > > > > Hi again,
> > > > > > >
> > > > > > > Taking a different approach to this.
> > > > > > >
> > > > > > > New webrev:
> > > > > > > http://cr.openjdk.java.net/~mlarsson/8146879/webrev.01/
> > > > > > >
> > > > > > > Existing files will now by default be renamed/archived with a .X suffix \
> > > > > > > where X is the lowest number such that the resulting file name is \
> > > > > > > available (jvm.log becomes jvm.log.0). A mode option for controlling \
> > > > > > > this behavior has been added as well. It can be set to archive, append, \
> > > > > > > or truncate (i.e. -Xlog::jvm.log::mode=truncate).
> > > > > > > Tested with included jtreg test through JPRT.
> > > > > > >
> > > > > > > Thanks,
> > > > > > > Marcus
> > > > > > >
> > > > > > > On 01/14/2016 04:00 PM, Marcus Larsson wrote:
> > > > > > > > Hi,
> > > > > > > >
> > > > > > > > Please review the following patch to make sure UL truncates existing \
> > > > > > > > log files before writing to them. Since files are opened in append \
> > > > > > > > mode, truncation isn't done automatically, so instead the patch adds \
> > > > > > > > an attempt to remove the log file before opening it.
> > > > > > > > Webrev:
> > > > > > > > http://cr.openjdk.java.net/~mlarsson/8146879/webrev.00/
> > > > > > > >
> > > > > > > > Issue:
> > > > > > > > https://bugs.openjdk.java.net/browse/JDK-8146879
> > > > > > > >
> > > > > > > > Testing:
> > > > > > > > Included test through JPRT
> > > > > > > >
> > > > > > > > Thanks,
> > > > > > > > Marcus
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic