Looks fine Ramanand. Please ensure the CCC is approved before pushing.
2 comments on the testcase :
fileHendlers --> fileHandlers.
Can you consider using the
jdk.testlibrary.FileUtils.deleteFileIfExistsWithRetry for removal of
directory that you create. It should be more stable in the long run.
Regards,
Sean.
On 17/06/16 12:19, Ramanand Patil wrote:
Hi All,
Gentle reminder...
Regards,
Ramanand.
-----Original Message-----
From: Ramanand Patil
Sent: Tuesday, June 14, 2016 9:51 PM
To: Daniel Fuchs; Bernd Eckenfels; core-libs-dev@openjdk.java.net
Subject: RE: RFR: 8153955: java.util.logging.FileHandler can not create file
synchronously over 101 access
Hi all,
May I request one more review for this bug fix.?
Thank you very much Daniel for your review.
Regards,
Ramanand.
-----Original Message-----
From: Daniel Fuchs
Sent: Friday, June 10, 2016 7:47 PM
To: Ramanand Patil; Bernd Eckenfels; core-libs-dev@openjdk.java.net
Subject: Re: RFR: 8153955: java.util.logging.FileHandler can not create file
synchronously over 101 access
Hi Ramanand,
Looks good to me now.
best regards,
-- daniel
On 10/06/16 14:47, Ramanand Patil wrote:
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.
