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