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.
> >>
> >
>
