Thank you Sean. Updated Webrev: http://cr.openjdk.java.net/~rpatil/8153955/webrev.03/
Regards, Ramanand. -----Original Message----- From: Seán Coffey Sent: Friday, June 17, 2016 9:34 PM To: Ramanand Patil; 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 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. >>>>>
