RFR: 8059767: FileHandler should allow 'long' limits and handle overflow of MeteredStream.written.
Hi, Please find below a patch for: 8059767: FileHandler should allow 'long' limits and handle overflow of MeteredStream.written. https://bugs.openjdk.java.net/browse/JDK-8059767 webrev: http://cr.openjdk.java.net/~dfuchs/webrev_8059767/webrev.00/ This follows an issue reported on this list: http://mail.openjdk.java.net/pipermail/core-libs-dev/2014-August/028280.html The patch changes 'limit' from 'int' to 'long', fixes the handling of overflow, and adds a new constructor that allows to pass a long for 'limit'. It also makes it possible to specify a long value for 'limit' in the configuration. The test checks the handling of overflow by tweaking the internal of MeteredStream through reflection. Not ideal, but I couldn't find any other way. best regards, -- daniel
Hi Daniel, The only thing I noticed is a missing @since tag on the FileHandler. Jason ----------------------------------------
Date: Tue, 7 Oct 2014 15:13:13 +0200 From: daniel.fuchs@oracle.com To: core-libs-dev@openjdk.java.net Subject: RFR: 8059767: FileHandler should allow 'long' limits and handle overflow of MeteredStream.written.
Hi,
Please find below a patch for:
8059767: FileHandler should allow 'long' limits and handle overflow of MeteredStream.written. https://bugs.openjdk.java.net/browse/JDK-8059767
webrev: http://cr.openjdk.java.net/~dfuchs/webrev_8059767/webrev.00/
This follows an issue reported on this list: http://mail.openjdk.java.net/pipermail/core-libs-dev/2014-August/028280.html
The patch changes 'limit' from 'int' to 'long', fixes the handling of overflow, and adds a new constructor that allows to pass a long for 'limit'. It also makes it possible to specify a long value for 'limit' in the configuration.
The test checks the handling of overflow by tweaking the internal of MeteredStream through reflection. Not ideal, but I couldn't find any other way.
best regards,
-- daniel
On 07/10/14 21:50, Jason Mehrens wrote:
Hi Daniel,
The only thing I noticed is a missing @since tag on the FileHandler.
Oops, thanks for catching that Jason. -- daniel
Jason
----------------------------------------
Date: Tue, 7 Oct 2014 15:13:13 +0200 From: daniel.fuchs@oracle.com To: core-libs-dev@openjdk.java.net Subject: RFR: 8059767: FileHandler should allow 'long' limits and handle overflow of MeteredStream.written.
Hi,
Please find below a patch for:
8059767: FileHandler should allow 'long' limits and handle overflow of MeteredStream.written. https://bugs.openjdk.java.net/browse/JDK-8059767
webrev: http://cr.openjdk.java.net/~dfuchs/webrev_8059767/webrev.00/
This follows an issue reported on this list: http://mail.openjdk.java.net/pipermail/core-libs-dev/2014-August/028280.html
The patch changes 'limit' from 'int' to 'long', fixes the handling of overflow, and adds a new constructor that allows to pass a long for 'limit'. It also makes it possible to specify a long value for 'limit' in the configuration.
The test checks the handling of overflow by tweaking the internal of MeteredStream through reflection. Not ideal, but I couldn't find any other way.
best regards,
-- daniel
Hi Daniel! Would it make sense to make the old constructor FileHandler(String, *int*, int, boolean) call the new one, casting the limit to long? Sincerely yours, Ivan On 07.10.2014 17:13, Daniel Fuchs wrote:
Hi,
Please find below a patch for:
8059767: FileHandler should allow 'long' limits and handle overflow of MeteredStream.written. https://bugs.openjdk.java.net/browse/JDK-8059767
webrev: http://cr.openjdk.java.net/~dfuchs/webrev_8059767/webrev.00/
This follows an issue reported on this list: http://mail.openjdk.java.net/pipermail/core-libs-dev/2014-August/028280.html
The patch changes 'limit' from 'int' to 'long', fixes the handling of overflow, and adds a new constructor that allows to pass a long for 'limit'. It also makes it possible to specify a long value for 'limit' in the configuration.
The test checks the handling of overflow by tweaking the internal of MeteredStream through reflection. Not ideal, but I couldn't find any other way.
best regards,
-- daniel
On 08/10/14 08:26, Ivan Gerasimov wrote:
Hi Daniel!
Would it make sense to make the old constructor FileHandler(String, *int*, int, boolean) call the new one, casting the limit to long?
Hi Ivan, I was about to reply 'no' out of consistency with the other constructors - which don't call each others - when I realized that it was not be possible to make the other constructors call each others anyway - as passing a parameter to a constructor means overriding the default value read from the configuration file. This however doesn't hold in our case as both constructors have exactly the same code - so there's no reason for not making FileHandler(String, *int*, int, boolean) call FileHandler(String, *long*, int, boolean). Which now makes me question whether I should add a second constructor FileHandler(String, *long*, int) to shadow FileHandler(String, *int*, int) - as Stanimir aleready suggested. My feeling is that the case where you would want to rely on the default value of 'append' configured in the logging.properties file instead of specifying one to the constructor should be sufficiently rare to not warrant the addition of a second constructor. And if you really wanted to rely on that default value you could go and fetch it before calling the constructor. Anyway - good remark - and here is a new webrev including yours and Jason's comments. http://cr.openjdk.java.net/~dfuchs/webrev_8059767/webrev.01 best regards, -- daniel
Sincerely yours, Ivan
On 07.10.2014 17:13, Daniel Fuchs wrote:
Hi,
Please find below a patch for:
8059767: FileHandler should allow 'long' limits and handle overflow of MeteredStream.written. https://bugs.openjdk.java.net/browse/JDK-8059767
webrev: http://cr.openjdk.java.net/~dfuchs/webrev_8059767/webrev.00/
This follows an issue reported on this list: http://mail.openjdk.java.net/pipermail/core-libs-dev/2014-August/028280.html
The patch changes 'limit' from 'int' to 'long', fixes the handling of overflow, and adds a new constructor that allows to pass a long for 'limit'. It also makes it possible to specify a long value for 'limit' in the configuration.
The test checks the handling of overflow by tweaking the internal of MeteredStream through reflection. Not ideal, but I couldn't find any other way.
best regards,
-- daniel
Hi, Following a feedback from Alan - I dropped 'SecurityException' from the throws clause of the new constructor - keeping only the @throws in the API documentation. Here is the new webrev (the above is the only change compared to the previous webrev.01). http://cr.openjdk.java.net/~dfuchs/webrev_8059767/webrev.02 I also logged JDK-8060457 to follow up and cleanup that in all handlers subclasses (no impact on the actual implementation / behavior - it's just cleanup). Attempting the cleanup in this fix was just too much noise. best regards, -- daniel On 08/10/14 11:46, Daniel Fuchs wrote:
On 08/10/14 08:26, Ivan Gerasimov wrote:
Hi Daniel!
Would it make sense to make the old constructor FileHandler(String, *int*, int, boolean) call the new one, casting the limit to long?
Hi Ivan,
I was about to reply 'no' out of consistency with the other constructors - which don't call each others - when I realized that it was not be possible to make the other constructors call each others anyway - as passing a parameter to a constructor means overriding the default value read from the configuration file.
This however doesn't hold in our case as both constructors have exactly the same code - so there's no reason for not making FileHandler(String, *int*, int, boolean) call FileHandler(String, *long*, int, boolean).
Which now makes me question whether I should add a second constructor FileHandler(String, *long*, int) to shadow FileHandler(String, *int*, int) - as Stanimir aleready suggested.
My feeling is that the case where you would want to rely on the default value of 'append' configured in the logging.properties file instead of specifying one to the constructor should be sufficiently rare to not warrant the addition of a second constructor. And if you really wanted to rely on that default value you could go and fetch it before calling the constructor.
Anyway - good remark - and here is a new webrev including yours and Jason's comments.
http://cr.openjdk.java.net/~dfuchs/webrev_8059767/webrev.01
best regards,
-- daniel
Sincerely yours, Ivan
On 07.10.2014 17:13, Daniel Fuchs wrote:
Hi,
Please find below a patch for:
8059767: FileHandler should allow 'long' limits and handle overflow of MeteredStream.written. https://bugs.openjdk.java.net/browse/JDK-8059767
webrev: http://cr.openjdk.java.net/~dfuchs/webrev_8059767/webrev.00/
This follows an issue reported on this list: http://mail.openjdk.java.net/pipermail/core-libs-dev/2014-August/028280.html
The patch changes 'limit' from 'int' to 'long', fixes the handling of overflow, and adds a new constructor that allows to pass a long for 'limit'. It also makes it possible to specify a long value for 'limit' in the configuration.
The test checks the handling of overflow by tweaking the internal of MeteredStream through reflection. Not ideal, but I couldn't find any other way.
best regards,
-- daniel
On 10/14/2014 3:55 AM, Daniel Fuchs wrote:
Looks good. Nit: use {@code...} instead of <tt>...</tt>. It may be inconsistent with the javadoc for other existing constructors that I think it's good to clean up the new one at least. No need to generate a new webrev. Mandy
participants (4)
-
Daniel Fuchs
-
Ivan Gerasimov
-
Jason Mehrens
-
Mandy Chung