Hi Daniel and Bernd,
Thank you for your reviews.
Here is the updated Webrev:
http://cr.openjdk.java.net/~rpatil/8153955/webrev.02/
Bernd,
Considering the information that "if the FileHandler tries to open the filename
and finds the file is currently in use by another process it will increment the
unique number field and try again" what you suggested looks correct. But using
concurrent/synchronous too should not make it wrong, because at this time all
the files are used simultaneously and any file which is not in use will be
opened and used by FileHandler. I think this should not make a big difference,
but as per your suggestion I have altered only that section of
logging.properties.
Test is also updated with the suggested comment. Thank you.
Daniel,
1. FileHandler.java: Changed the wordings as you suggested.
2. FileHandlerMaxLocksTest::main
- Now using "user.dir" in createLoggerDir(). I saw some tests(like
java/util/logging/FileHandlerPath.java) were already using user home or temp
directory, so thought that should not be a problem.
- Removed the WeekReference and using ArrayList for all the FileHandler
instances.
- Tested on Windows, Linux(Ubuntu14.04) and Solaris and test runs
fine(able to delete all the log files and loggerDir). Hopefully, test will not
have any issues on other platforms.(JPRT job is also passed).
Regards,
Ramanand.
-----Original Message-----
From: Bernd Eckenfels [mailto:e...@zusammenkunft.net]
Sent: Thursday, June 09, 2016 11:16 PM
To: Daniel Fuchs; core-libs-dev@openjdk.java.net
Subject: Re: RFR: 8153955: java.util.logging.FileHandler can not create file
synchronously over 101 access
Hello,
I find the "concurrent/synchronous" comment a bit confusing.
How about:
# Number of attempts to obtain lock file in FileHandler # Implemented by
incrementing the %u placeholder as documented # in FileHandler Javadoc
In the test I would add a comment
"200 raises the default limit of 100, we try 102 times"
Gruss
Bernd
Am Thu, 9 Jun 2016 07:31:25 +0100
schrieb Daniel Fuchs <daniel.fu...@oracle.com>:
> Hi Ramanand,
>
> Thanks for the updated. I still have some remarks:
>
> FileHandler.java:
>
> 98 * specifies the maximum number of concurrent locks hold
> by 99 * FileHandler (defaults 100). </li>
>
> Is the verb form correct: hold => held ?
>
> logging.properties:
>
> 42 # when the unique field %u is incremented as per the javadoc.
>
> "... as per the javadoc" => "... as per FileHandler API documentation"
>
> FileHandlerMaxLocksTest.java:
>
> - FileHandlerMaxLocksTest::createLoggerDir():
>
> 75 String tmpDir = System.getProperty("java.io.tmpdir");
> 76 if (tmpDir == null) {
> 77 tmpDir = System.getProperty("user.home");
> 78 }
> 79 File tmpOrHomeDir = new File(tmpDir);
> 80 File loggerDir = new File(tmpOrHomeDir, LOGGER_DIR);
>
> The preferred place for a test to create file is the scratch directory
> that jtreg creates for the test. The scratch directory location is
> available from the "user.dir" system property.
>
> I suggest changing the code above to:
>
> String userDir = System.getProperty("user.dir", ".");
> File loggerDir = new File(userDir, LOGGER_DIR);
>
> - FileHandlerMaxLocksTest::main
>
> I am not sure what you try to achieve by using WeakReference there.
> I would suggest to store all created FileHandler instances into a
> list, which would allow you to close them properly in the finally
> clause just before deleting LOGGER_DIR.
>
> Different operating systems might handle lock files differently.
> I am afraid that some operating system might not let you delete a
> directory that contains a file which is still locked by the system.
>
> best regards,
>
> -- daniel
>
> On 09/06/16 06:40, Ramanand Patil wrote:
> > Hi,
> >
> > Please review the updated Webrev at:
> > http://cr.openjdk.java.net/~rpatil/8153955/webrev.01/
> >
> > FileHander.java and logging.properties files are updated as per
> > Daniel's suggestion.
> >
> >
> > Regards,
> > Ramanand.
> >
> >
> > -----Original Message-----
> > From: Daniel Fuchs
> > Sent: Wednesday, June 08, 2016 6:57 PM
> > To: Ramanand Patil; core-libs-dev@openjdk.java.net
> > Subject: Re: RFR: 8153955: java.util.logging.FileHandler can not
> > create file synchronously over 101 access
> >
> > Hi Ramanand,
> >
> > Thanks for looking into this.
> >
> > 1. FileHander.java:
> >
> > 94 * <li> <handler-name>.append
> > 95 * specifies whether the FileHandler should append onto
> > 96 * any existing files (defaults to false). </li>
> > 97 * <li> java.util.FileHandler.maxLocks
> > 98 * specifies the maximum number of concurrent locks
> > hold by 99 * FileHandler (defaults 100). </li>
> >
> > line 97: a) this is java.util.logging.FileHandler, not
> > java.util.FileHandler. b) for consistency with the specification of
> > other FileHandler properties I suggest to change this line to:
> >
> > 97 * <li> <handler-name>.maxLock
> >
> > 2. logging.properties:
> >
> > 31 # Default number of locks FileHandler can obtain
> > synchronously. 32 # This specifies maximum number of concurrent
> > locks by FileHandler 33 # when the unique field %u is incremented as
> > per the javadoc. 34 java.util.logging.FileHandler.maxLocks = 100
> >
> > Please move lines 31-34 between lines 44 and 45: that's where they
> > belong.
> >
> > best regards,
> >
> > -- daniel
> >
> >
> >
> >
> > On 08/06/16 12:06, Ramanand Patil wrote:
> >> Hi all,
> >>
> >> Please review the following bug fix:
> >>
> >> Bug: https://bugs.openjdk.java.net/browse/JDK-8153955
> >>
> >> Webrev: http://cr.openjdk.java.net/~rpatil/8153955/webrev.00/
> >>
> >> Fix: A new configurable java property-
> >> "java.util.logging.FileHandler.maxLocks" is added to the
> >> FileHandler which can be set in the custom logger config file. The
> >> default value of the this property will be mentioned in the default
> >> config file.(logging.properties).
> >>
> >>
> >>
> >>
> >>
> >> Regards,
> >>
> >> Ramanand.
> >>
> >
>
