You are viewing a plain text version of this content. The canonical link for it is here.
Posted to log4j-dev@logging.apache.org by Gary Gregory <ga...@gmail.com> on 2016/06/23 23:23:37 UTC
AbstractManager.release() -> AutoCloseable
I seems that AbstractManager should implement AutoCloseable where close()
does the same thing as release() and release() can be deprecated.
org.apache.logging.log4j.core.appender.AbstractManager.release()
This will let us rewrite things like the recently fixed
"OnStartupTriggeringPolicyTest
fails on Windows saying the file is used by another process"
https://issues.apache.org/jira/browse/LOG4J2-1445:
Is:
final RollingFileManager manager =
RollingFileManager.getFileManager(TARGET_FILE, TARGET_PATTERN, true, false,
policy, strategy, null, layout, 8192, true);
try {
manager.initialize();
assertTrue(Files.exists(target));
assertTrue(Files.size(target) == 0);
assertTrue(Files.exists(rolled));
assertTrue(Files.size(rolled) == size);
} finally {
manager.release();
}
Could be:
try (final RollingFileManager manager =
RollingFileManager.getFileManager(TARGET_FILE, TARGET_PATTERN, true, false,
policy, strategy, null, layout, 8192, true)) {
manager.initialize();
assertTrue(Files.exists(target));
assertTrue(Files.size(target) == 0);
assertTrue(Files.exists(rolled));
assertTrue(Files.size(rolled) == size);
}
Thoughts?
Gary
--
E-Mail: garydgregory@gmail.com | ggregory@apache.org
Java Persistence with Hibernate, Second Edition
<http://www.manning.com/bauer3/>
JUnit in Action, Second Edition <http://www.manning.com/tahchiev/>
Spring Batch in Action <http://www.manning.com/templier/>
Blog: http://garygregory.wordpress.com
Home: http://garygregory.com/
Tweet! http://twitter.com/GaryGregory
Re: AbstractManager.release() -> AutoCloseable
Posted by Remko Popma <re...@gmail.com>.
Interesting. On the plus side tests that declared the Manager inside the try won't have to worry about calling release().
On the minus side, in actual (non-test) usage, Managers are long-lived objects that are not supposed to be created and destroyed within the scope of a single method call. So declaring them as Closeable might be slightly confusing.
I don't consider that potential confusion a major drawback though, as long as we document that Closeable is only implemented to facilitate test cleanup.
No strong opinion either way.
Remko
Sent from my iPhone
> On 2016/06/29, at 7:52, Gary Gregory <ga...@gmail.com> wrote:
>
> Would anyone else care to opine?
>
> G
>
>> On Tue, Jun 28, 2016 at 1:20 AM, Mikael Ståldal <mi...@magine.com> wrote:
>> Sounds good.
>>
>>> On Fri, Jun 24, 2016 at 1:23 AM, Gary Gregory <ga...@gmail.com> wrote:
>>> I seems that AbstractManager should implement AutoCloseable where close() does the same thing as release() and release() can be deprecated.
>>>
>>> org.apache.logging.log4j.core.appender.AbstractManager.release()
>>>
>>> This will let us rewrite things like the recently fixed "OnStartupTriggeringPolicyTest fails on Windows saying the file is used by another process" https://issues.apache.org/jira/browse/LOG4J2-1445:
>>>
>>> Is:
>>>
>>> final RollingFileManager manager = RollingFileManager.getFileManager(TARGET_FILE, TARGET_PATTERN, true, false,
>>> policy, strategy, null, layout, 8192, true);
>>> try {
>>> manager.initialize();
>>> assertTrue(Files.exists(target));
>>> assertTrue(Files.size(target) == 0);
>>> assertTrue(Files.exists(rolled));
>>> assertTrue(Files.size(rolled) == size);
>>> } finally {
>>> manager.release();
>>> }
>>>
>>> Could be:
>>>
>>> try (final RollingFileManager manager = RollingFileManager.getFileManager(TARGET_FILE, TARGET_PATTERN, true, false,
>>> policy, strategy, null, layout, 8192, true)) {
>>> manager.initialize();
>>> assertTrue(Files.exists(target));
>>> assertTrue(Files.size(target) == 0);
>>> assertTrue(Files.exists(rolled));
>>> assertTrue(Files.size(rolled) == size);
>>> }
>>>
>>> Thoughts?
>>>
>>> Gary
>>>
>>>
>>> --
>>> E-Mail: garydgregory@gmail.com | ggregory@apache.org
>>> Java Persistence with Hibernate, Second Edition
>>> JUnit in Action, Second Edition
>>> Spring Batch in Action
>>> Blog: http://garygregory.wordpress.com
>>> Home: http://garygregory.com/
>>> Tweet! http://twitter.com/GaryGregory
>>
>>
>>
>> --
>>
>>
>> Mikael Ståldal
>> Senior software developer
>>
>> Magine TV
>> mikael.staldal@magine.com
>> Grev Turegatan 3 | 114 46 Stockholm, Sweden | www.magine.com
>>
>> Privileged and/or Confidential Information may be contained in this message. If you are not the addressee indicated in this message
>> (or responsible for delivery of the message to such a person), you may not copy or deliver this message to anyone. In such case,
>> you should destroy this message and kindly notify the sender by reply email.
>
>
>
> --
> E-Mail: garydgregory@gmail.com | ggregory@apache.org
> Java Persistence with Hibernate, Second Edition
> JUnit in Action, Second Edition
> Spring Batch in Action
> Blog: http://garygregory.wordpress.com
> Home: http://garygregory.com/
> Tweet! http://twitter.com/GaryGregory
Re: AbstractManager.release() -> AutoCloseable
Posted by Ralph Goers <ra...@dslextreme.com>.
I have nothing against it. But since it isn’t a public API it wouldn’t be a priority for me. There are other far more interesting problems to solve (many of which are in the 300 some open Jira issues we have).
Ralph
> On Jun 28, 2016, at 3:52 PM, Gary Gregory <ga...@gmail.com> wrote:
>
> Would anyone else care to opine?
>
> G
>
> On Tue, Jun 28, 2016 at 1:20 AM, Mikael Ståldal <mikael.staldal@magine.com <ma...@magine.com>> wrote:
> Sounds good.
>
> On Fri, Jun 24, 2016 at 1:23 AM, Gary Gregory <garydgregory@gmail.com <ma...@gmail.com>> wrote:
> I seems that AbstractManager should implement AutoCloseable where close() does the same thing as release() and release() can be deprecated.
>
> org.apache.logging.log4j.core.appender.AbstractManager.release()
>
> This will let us rewrite things like the recently fixed "OnStartupTriggeringPolicyTest fails on Windows saying the file is used by another process" https://issues.apache.org/jira/browse/LOG4J2-1445 <https://issues.apache.org/jira/browse/LOG4J2-1445>:
>
> Is:
>
> final RollingFileManager manager = RollingFileManager.getFileManager(TARGET_FILE, TARGET_PATTERN, true, false,
> policy, strategy, null, layout, 8192, true);
> try {
> manager.initialize();
> assertTrue(Files.exists(target));
> assertTrue(Files.size(target) == 0);
> assertTrue(Files.exists(rolled));
> assertTrue(Files.size(rolled) == size);
> } finally {
> manager.release();
> }
>
> Could be:
>
> try (final RollingFileManager manager = RollingFileManager.getFileManager(TARGET_FILE, TARGET_PATTERN, true, false,
> policy, strategy, null, layout, 8192, true)) {
> manager.initialize();
> assertTrue(Files.exists(target));
> assertTrue(Files.size(target) == 0);
> assertTrue(Files.exists(rolled));
> assertTrue(Files.size(rolled) == size);
> }
>
> Thoughts?
>
> Gary
>
>
> --
> E-Mail: garydgregory@gmail.com <ma...@gmail.com> | ggregory@apache.org <ma...@apache.org>
> Java Persistence with Hibernate, Second Edition <http://www.manning.com/bauer3/>
> JUnit in Action, Second Edition <http://www.manning.com/tahchiev/>
> Spring Batch in Action <http://www.manning.com/templier/>
> Blog: http://garygregory.wordpress.com <http://garygregory.wordpress.com/>
> Home: http://garygregory.com/ <http://garygregory.com/>
> Tweet! http://twitter.com/GaryGregory <http://twitter.com/GaryGregory>
>
>
> --
>
>
> Mikael Ståldal
> Senior software developer
>
> Magine TV
> mikael.staldal@magine.com <ma...@magine.com>
> Grev Turegatan 3 | 114 46 Stockholm, Sweden | www.magine.com <http://www.magine.com/>
>
> Privileged and/or Confidential Information may be contained in this message. If you are not the addressee indicated in this message
> (or responsible for delivery of the message to such a person), you may not copy or deliver this message to anyone. In such case,
> you should destroy this message and kindly notify the sender by reply email.
>
>
>
> --
> E-Mail: garydgregory@gmail.com <ma...@gmail.com> | ggregory@apache.org <ma...@apache.org>
> Java Persistence with Hibernate, Second Edition <http://www.manning.com/bauer3/>
> JUnit in Action, Second Edition <http://www.manning.com/tahchiev/>
> Spring Batch in Action <http://www.manning.com/templier/>
> Blog: http://garygregory.wordpress.com <http://garygregory.wordpress.com/>
> Home: http://garygregory.com/ <http://garygregory.com/>
> Tweet! http://twitter.com/GaryGregory <http://twitter.com/GaryGregory>
Re: AbstractManager.release() -> AutoCloseable
Posted by Gary Gregory <ga...@gmail.com>.
Would anyone else care to opine?
G
On Tue, Jun 28, 2016 at 1:20 AM, Mikael Ståldal <mi...@magine.com>
wrote:
> Sounds good.
>
> On Fri, Jun 24, 2016 at 1:23 AM, Gary Gregory <ga...@gmail.com>
> wrote:
>
>> I seems that AbstractManager should implement AutoCloseable where close()
>> does the same thing as release() and release() can be deprecated.
>>
>> org.apache.logging.log4j.core.appender.AbstractManager.release()
>>
>> This will let us rewrite things like the recently fixed "OnStartupTriggeringPolicyTest
>> fails on Windows saying the file is used by another process"
>> https://issues.apache.org/jira/browse/LOG4J2-1445:
>>
>> Is:
>>
>> final RollingFileManager manager =
>> RollingFileManager.getFileManager(TARGET_FILE, TARGET_PATTERN, true, false,
>> policy, strategy, null, layout, 8192, true);
>> try {
>> manager.initialize();
>> assertTrue(Files.exists(target));
>> assertTrue(Files.size(target) == 0);
>> assertTrue(Files.exists(rolled));
>> assertTrue(Files.size(rolled) == size);
>> } finally {
>> manager.release();
>> }
>>
>> Could be:
>>
>> try (final RollingFileManager manager =
>> RollingFileManager.getFileManager(TARGET_FILE, TARGET_PATTERN, true, false,
>> policy, strategy, null, layout, 8192, true)) {
>> manager.initialize();
>> assertTrue(Files.exists(target));
>> assertTrue(Files.size(target) == 0);
>> assertTrue(Files.exists(rolled));
>> assertTrue(Files.size(rolled) == size);
>> }
>>
>> Thoughts?
>>
>> Gary
>>
>>
>> --
>> E-Mail: garydgregory@gmail.com | ggregory@apache.org
>> Java Persistence with Hibernate, Second Edition
>> <http://www.manning.com/bauer3/>
>> JUnit in Action, Second Edition <http://www.manning.com/tahchiev/>
>> Spring Batch in Action <http://www.manning.com/templier/>
>> Blog: http://garygregory.wordpress.com
>> Home: http://garygregory.com/
>> Tweet! http://twitter.com/GaryGregory
>>
>
>
>
> --
> [image: MagineTV]
>
> *Mikael Ståldal*
> Senior software developer
>
> *Magine TV*
> mikael.staldal@magine.com
> Grev Turegatan 3 | 114 46 Stockholm, Sweden | www.magine.com
>
> Privileged and/or Confidential Information may be contained in this
> message. If you are not the addressee indicated in this message
> (or responsible for delivery of the message to such a person), you may not
> copy or deliver this message to anyone. In such case,
> you should destroy this message and kindly notify the sender by reply
> email.
>
--
E-Mail: garydgregory@gmail.com | ggregory@apache.org
Java Persistence with Hibernate, Second Edition
<http://www.manning.com/bauer3/>
JUnit in Action, Second Edition <http://www.manning.com/tahchiev/>
Spring Batch in Action <http://www.manning.com/templier/>
Blog: http://garygregory.wordpress.com
Home: http://garygregory.com/
Tweet! http://twitter.com/GaryGregory
Re: AbstractManager.release() -> AutoCloseable
Posted by Mikael Ståldal <mi...@magine.com>.
Sounds good.
On Fri, Jun 24, 2016 at 1:23 AM, Gary Gregory <ga...@gmail.com>
wrote:
> I seems that AbstractManager should implement AutoCloseable where close()
> does the same thing as release() and release() can be deprecated.
>
> org.apache.logging.log4j.core.appender.AbstractManager.release()
>
> This will let us rewrite things like the recently fixed "OnStartupTriggeringPolicyTest
> fails on Windows saying the file is used by another process"
> https://issues.apache.org/jira/browse/LOG4J2-1445:
>
> Is:
>
> final RollingFileManager manager =
> RollingFileManager.getFileManager(TARGET_FILE, TARGET_PATTERN, true, false,
> policy, strategy, null, layout, 8192, true);
> try {
> manager.initialize();
> assertTrue(Files.exists(target));
> assertTrue(Files.size(target) == 0);
> assertTrue(Files.exists(rolled));
> assertTrue(Files.size(rolled) == size);
> } finally {
> manager.release();
> }
>
> Could be:
>
> try (final RollingFileManager manager =
> RollingFileManager.getFileManager(TARGET_FILE, TARGET_PATTERN, true, false,
> policy, strategy, null, layout, 8192, true)) {
> manager.initialize();
> assertTrue(Files.exists(target));
> assertTrue(Files.size(target) == 0);
> assertTrue(Files.exists(rolled));
> assertTrue(Files.size(rolled) == size);
> }
>
> Thoughts?
>
> Gary
>
>
> --
> E-Mail: garydgregory@gmail.com | ggregory@apache.org
> Java Persistence with Hibernate, Second Edition
> <http://www.manning.com/bauer3/>
> JUnit in Action, Second Edition <http://www.manning.com/tahchiev/>
> Spring Batch in Action <http://www.manning.com/templier/>
> Blog: http://garygregory.wordpress.com
> Home: http://garygregory.com/
> Tweet! http://twitter.com/GaryGregory
>
--
[image: MagineTV]
*Mikael Ståldal*
Senior software developer
*Magine TV*
mikael.staldal@magine.com
Grev Turegatan 3 | 114 46 Stockholm, Sweden | www.magine.com
Privileged and/or Confidential Information may be contained in this
message. If you are not the addressee indicated in this message
(or responsible for delivery of the message to such a person), you may not
copy or deliver this message to anyone. In such case,
you should destroy this message and kindly notify the sender by reply
email.