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 Remko Popma <re...@gmail.com> on 2016/08/28 23:01:08 UTC
Re: [1/4] logging-log4j2 git commit: [LOG4J2-1553] AbstractManager
should implement AutoCloseable. Rename AbstractManager.close() to clone{Resource}().
I don't think the close() method should be renamed to closeOutputStream(),
at least for
MemoryMappedFileManager,
RandomAccessFileManager, and
RollingRandomAccessFileManager.
The above three have a dummy OutputStream because they inherit from
OuputStreamManager, but closing this stream does not do much. Naming the
method such would be emphasizing the wrong thing.
Other Managers may manage yet other resources.
What is wrong with a generic close() method?
Also, you state AbstractManager should implement AutoCloseable
without any indication why. There must be some reason why you think so.
Please share it.
On Mon, Aug 29, 2016 at 5:22 AM, <gg...@apache.org> wrote:
> Repository: logging-log4j2
> Updated Branches:
> refs/heads/LOG4J2-1553 [created] 13c3c2479
>
>
> [LOG4J2-1553] AbstractManager should implement AutoCloseable. Rename
> AbstractManager.close() to clone{Resource}().
>
> Project: http://git-wip-us.apache.org/repos/asf/logging-log4j2/repo
> Commit: http://git-wip-us.apache.org/repos/asf/logging-log4j2/
> commit/b73c589c
> Tree: http://git-wip-us.apache.org/repos/asf/logging-log4j2/tree/b73c589c
> Diff: http://git-wip-us.apache.org/repos/asf/logging-log4j2/diff/b73c589c
>
> Branch: refs/heads/LOG4J2-1553
> Commit: b73c589c72bd526846ce045c10e5146212a96c88
> Parents: f936a6d
> Author: Gary Gregory <gg...@apache.org>
> Authored: Sun Aug 28 12:04:55 2016 -0700
> Committer: Gary Gregory <gg...@apache.org>
> Committed: Sun Aug 28 12:04:55 2016 -0700
>
> ----------------------------------------------------------------------
> .../logging/log4j/core/appender/MemoryMappedFileManager.java | 2 +-
> .../apache/logging/log4j/core/appender/OutputStreamManager.java | 4 ++--
> .../logging/log4j/core/appender/RandomAccessFileManager.java | 2 +-
> .../org/apache/logging/log4j/core/appender/WriterManager.java | 4 ++--
> .../logging/log4j/core/appender/rolling/RollingFileManager.java | 2 +-
> .../core/appender/rolling/RollingRandomAccessFileManager.java | 2 +-
> .../java/org/apache/logging/log4j/core/net/TcpSocketManager.java | 4 ++--
> 7 files changed, 10 insertions(+), 10 deletions(-)
> ----------------------------------------------------------------------
>
>
> http://git-wip-us.apache.org/repos/asf/logging-log4j2/blob/
> b73c589c/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/
> MemoryMappedFileManager.java
> ----------------------------------------------------------------------
> diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/
> appender/MemoryMappedFileManager.java b/log4j-core/src/main/java/
> org/apache/logging/log4j/core/appender/MemoryMappedFileManager.java
> index e238258..a856610 100644
> --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/
> MemoryMappedFileManager.java
> +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/
> MemoryMappedFileManager.java
> @@ -156,7 +156,7 @@ public class MemoryMappedFileManager extends
> OutputStreamManager {
> }
>
> @Override
> - public synchronized void close() {
> + public synchronized void closeOutputStream() {
> final long position = mappedBuffer.position();
> final long length = mappingOffset + position;
> try {
>
> http://git-wip-us.apache.org/repos/asf/logging-log4j2/blob/
> b73c589c/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/
> OutputStreamManager.java
> ----------------------------------------------------------------------
> diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/
> appender/OutputStreamManager.java b/log4j-core/src/main/java/
> org/apache/logging/log4j/core/appender/OutputStreamManager.java
> index 698409a..97a9dc6 100644
> --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/
> appender/OutputStreamManager.java
> +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/
> appender/OutputStreamManager.java
> @@ -126,7 +126,7 @@ public class OutputStreamManager extends
> AbstractManager implements ByteBufferDe
> @Override
> public void releaseSub() {
> writeFooter();
> - close();
> + closeOutputStream();
> }
>
> /**
> @@ -287,7 +287,7 @@ public class OutputStreamManager extends
> AbstractManager implements ByteBufferDe
> flushDestination();
> }
>
> - protected synchronized void close() {
> + protected synchronized void closeOutputStream() {
> flush();
> final OutputStream stream = os; // access volatile field only
> once per method
> if (stream == null || stream == System.out || stream ==
> System.err) {
>
> http://git-wip-us.apache.org/repos/asf/logging-log4j2/blob/
> b73c589c/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/
> RandomAccessFileManager.java
> ----------------------------------------------------------------------
> diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/
> appender/RandomAccessFileManager.java b/log4j-core/src/main/java/
> org/apache/logging/log4j/core/appender/RandomAccessFileManager.java
> index 7ab0fe3..6aca266 100644
> --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/
> RandomAccessFileManager.java
> +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/
> RandomAccessFileManager.java
> @@ -98,7 +98,7 @@ public class RandomAccessFileManager extends
> OutputStreamManager {
> }
>
> @Override
> - public synchronized void close() {
> + public synchronized void closeOutputStream() {
> flush();
> try {
> randomAccessFile.close();
>
> http://git-wip-us.apache.org/repos/asf/logging-log4j2/blob/
> b73c589c/log4j-core/src/main/java/org/apache/logging/log4j/
> core/appender/WriterManager.java
> ----------------------------------------------------------------------
> diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/WriterManager.java
> b/log4j-core/src/main/java/org/apache/logging/log4j/core/
> appender/WriterManager.java
> index b3b9e73..cee99e8 100644
> --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/
> appender/WriterManager.java
> +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/
> appender/WriterManager.java
> @@ -61,7 +61,7 @@ public class WriterManager extends AbstractManager {
> }
> }
>
> - protected synchronized void close() {
> + protected synchronized void closeWriter() {
> final Writer w = writer; // access volatile field only once per
> method
> try {
> w.close();
> @@ -100,7 +100,7 @@ public class WriterManager extends AbstractManager {
> @Override
> public void releaseSub() {
> writeFooter();
> - close();
> + closeWriter();
> }
>
> protected void setWriter(final Writer writer) {
>
> http://git-wip-us.apache.org/repos/asf/logging-log4j2/blob/
> b73c589c/log4j-core/src/main/java/org/apache/logging/log4j/
> core/appender/rolling/RollingFileManager.java
> ----------------------------------------------------------------------
> diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/
> appender/rolling/RollingFileManager.java b/log4j-core/src/main/java/
> org/apache/logging/log4j/core/appender/rolling/RollingFileManager.java
> index 3f8ba2a..e00319d 100644
> --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/
> appender/rolling/RollingFileManager.java
> +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/
> appender/rolling/RollingFileManager.java
> @@ -242,7 +242,7 @@ public class RollingFileManager extends FileManager {
> final RolloverDescription descriptor =
> strategy.rollover(this);
> if (descriptor != null) {
> writeFooter();
> - close();
> + closeOutputStream();
> if (descriptor.getSynchronous() != null) {
> LOGGER.debug("RollingFileManager executing
> synchronous {}", descriptor.getSynchronous());
> try {
>
> http://git-wip-us.apache.org/repos/asf/logging-log4j2/blob/
> b73c589c/log4j-core/src/main/java/org/apache/logging/log4j/
> core/appender/rolling/RollingRandomAccessFileManager.java
> ----------------------------------------------------------------------
> diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/
> appender/rolling/RollingRandomAccessFileManager.java
> b/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/rolling/
> RollingRandomAccessFileManager.java
> index 8b39209..f27d445 100644
> --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/
> appender/rolling/RollingRandomAccessFileManager.java
> +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/
> appender/rolling/RollingRandomAccessFileManager.java
> @@ -128,7 +128,7 @@ public class RollingRandomAccessFileManager extends
> RollingFileManager {
> }
>
> @Override
> - public synchronized void close() {
> + public synchronized void closeOutputStream() {
> flush();
> try {
> randomAccessFile.close();
>
> http://git-wip-us.apache.org/repos/asf/logging-log4j2/blob/
> b73c589c/log4j-core/src/main/java/org/apache/logging/log4j/
> core/net/TcpSocketManager.java
> ----------------------------------------------------------------------
> diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/net/TcpSocketManager.java
> b/log4j-core/src/main/java/org/apache/logging/log4j/core/
> net/TcpSocketManager.java
> index 4d83d97..a34ce30 100644
> --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/
> net/TcpSocketManager.java
> +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/
> net/TcpSocketManager.java
> @@ -147,8 +147,8 @@ public class TcpSocketManager extends
> AbstractSocketManager {
> }
>
> @Override
> - protected synchronized void close() {
> - super.close();
> + protected synchronized void closeOutputStream() {
> + super.closeOutputStream();
> if (connector != null) {
> connector.shutdown();
> connector.interrupt();
>
>
Re: [1/4] logging-log4j2 git commit: [LOG4J2-1553] AbstractManager
should implement AutoCloseable. Rename AbstractManager.close() to clone{Resource}().
Posted by Gary Gregory <ga...@gmail.com>.
On Sun, Aug 28, 2016 at 4:01 PM, Remko Popma <re...@gmail.com> wrote:
> I don't think the close() method should be renamed to closeOutputStream(),
> at least for
> MemoryMappedFileManager,
> RandomAccessFileManager, and
> RollingRandomAccessFileManager.
>
> The above three have a dummy OutputStream because they inherit from
> OuputStreamManager, but closing this stream does not do much. Naming the
> method such would be emphasizing the wrong thing.
>
Hi Remko,
You could argue it the other way around, it is the fact that these appender
extend OuputStreamManager without really using an output stream that is odd
and/or misleading. This seems a bigger oddity than the name of a method,
>
> Other Managers may manage yet other resources.
> What is wrong with a generic close() method?
>
There is a close method but it is a method that is really implemented by
the releaseSub() method.
>
> Also, you state AbstractManager should implement AutoCloseable
> without any indication why. There must be some reason why you think so.
> Please share it.
>
I updated the ticket and the Javadoc.
Thank you,
Gary
>
>
>
> On Mon, Aug 29, 2016 at 5:22 AM, <gg...@apache.org> wrote:
>
>> Repository: logging-log4j2
>> Updated Branches:
>> refs/heads/LOG4J2-1553 [created] 13c3c2479
>>
>>
>> [LOG4J2-1553] AbstractManager should implement AutoCloseable. Rename
>> AbstractManager.close() to clone{Resource}().
>>
>> Project: http://git-wip-us.apache.org/repos/asf/logging-log4j2/repo
>> Commit: http://git-wip-us.apache.org/repos/asf/logging-log4j2/commit
>> /b73c589c
>> Tree: http://git-wip-us.apache.org/repos/asf/logging-log4j2/tree/b73c589c
>> Diff: http://git-wip-us.apache.org/repos/asf/logging-log4j2/diff/b73c589c
>>
>> Branch: refs/heads/LOG4J2-1553
>> Commit: b73c589c72bd526846ce045c10e5146212a96c88
>> Parents: f936a6d
>> Author: Gary Gregory <gg...@apache.org>
>> Authored: Sun Aug 28 12:04:55 2016 -0700
>> Committer: Gary Gregory <gg...@apache.org>
>> Committed: Sun Aug 28 12:04:55 2016 -0700
>>
>> ----------------------------------------------------------------------
>> .../logging/log4j/core/appender/MemoryMappedFileManager.java | 2 +-
>> .../apache/logging/log4j/core/appender/OutputStreamManager.java | 4
>> ++--
>> .../logging/log4j/core/appender/RandomAccessFileManager.java | 2 +-
>> .../org/apache/logging/log4j/core/appender/WriterManager.java | 4
>> ++--
>> .../logging/log4j/core/appender/rolling/RollingFileManager.java | 2 +-
>> .../core/appender/rolling/RollingRandomAccessFileManager.java | 2 +-
>> .../java/org/apache/logging/log4j/core/net/TcpSocketManager.java | 4
>> ++--
>> 7 files changed, 10 insertions(+), 10 deletions(-)
>> ----------------------------------------------------------------------
>>
>>
>> http://git-wip-us.apache.org/repos/asf/logging-log4j2/blob/b
>> 73c589c/log4j-core/src/main/java/org/apache/logging/log4j/co
>> re/appender/MemoryMappedFileManager.java
>> ----------------------------------------------------------------------
>> diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/app
>> ender/MemoryMappedFileManager.java b/log4j-core/src/main/java/org
>> /apache/logging/log4j/core/appender/MemoryMappedFileManager.java
>> index e238258..a856610 100644
>> --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/app
>> ender/MemoryMappedFileManager.java
>> +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/app
>> ender/MemoryMappedFileManager.java
>> @@ -156,7 +156,7 @@ public class MemoryMappedFileManager extends
>> OutputStreamManager {
>> }
>>
>> @Override
>> - public synchronized void close() {
>> + public synchronized void closeOutputStream() {
>> final long position = mappedBuffer.position();
>> final long length = mappingOffset + position;
>> try {
>>
>> http://git-wip-us.apache.org/repos/asf/logging-log4j2/blob/b
>> 73c589c/log4j-core/src/main/java/org/apache/logging/log4j/co
>> re/appender/OutputStreamManager.java
>> ----------------------------------------------------------------------
>> diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/OutputStreamManager.java
>> b/log4j-core/src/main/java/org/apache/logging/log4j/core/app
>> ender/OutputStreamManager.java
>> index 698409a..97a9dc6 100644
>> --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/app
>> ender/OutputStreamManager.java
>> +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/app
>> ender/OutputStreamManager.java
>> @@ -126,7 +126,7 @@ public class OutputStreamManager extends
>> AbstractManager implements ByteBufferDe
>> @Override
>> public void releaseSub() {
>> writeFooter();
>> - close();
>> + closeOutputStream();
>> }
>>
>> /**
>> @@ -287,7 +287,7 @@ public class OutputStreamManager extends
>> AbstractManager implements ByteBufferDe
>> flushDestination();
>> }
>>
>> - protected synchronized void close() {
>> + protected synchronized void closeOutputStream() {
>> flush();
>> final OutputStream stream = os; // access volatile field only
>> once per method
>> if (stream == null || stream == System.out || stream ==
>> System.err) {
>>
>> http://git-wip-us.apache.org/repos/asf/logging-log4j2/blob/b
>> 73c589c/log4j-core/src/main/java/org/apache/logging/log4j/co
>> re/appender/RandomAccessFileManager.java
>> ----------------------------------------------------------------------
>> diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/app
>> ender/RandomAccessFileManager.java b/log4j-core/src/main/java/org
>> /apache/logging/log4j/core/appender/RandomAccessFileManager.java
>> index 7ab0fe3..6aca266 100644
>> --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/app
>> ender/RandomAccessFileManager.java
>> +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/app
>> ender/RandomAccessFileManager.java
>> @@ -98,7 +98,7 @@ public class RandomAccessFileManager extends
>> OutputStreamManager {
>> }
>>
>> @Override
>> - public synchronized void close() {
>> + public synchronized void closeOutputStream() {
>> flush();
>> try {
>> randomAccessFile.close();
>>
>> http://git-wip-us.apache.org/repos/asf/logging-log4j2/blob/b
>> 73c589c/log4j-core/src/main/java/org/apache/logging/log4j/co
>> re/appender/WriterManager.java
>> ----------------------------------------------------------------------
>> diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/WriterManager.java
>> b/log4j-core/src/main/java/org/apache/logging/log4j/core/app
>> ender/WriterManager.java
>> index b3b9e73..cee99e8 100644
>> --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/app
>> ender/WriterManager.java
>> +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/app
>> ender/WriterManager.java
>> @@ -61,7 +61,7 @@ public class WriterManager extends AbstractManager {
>> }
>> }
>>
>> - protected synchronized void close() {
>> + protected synchronized void closeWriter() {
>> final Writer w = writer; // access volatile field only once per
>> method
>> try {
>> w.close();
>> @@ -100,7 +100,7 @@ public class WriterManager extends AbstractManager {
>> @Override
>> public void releaseSub() {
>> writeFooter();
>> - close();
>> + closeWriter();
>> }
>>
>> protected void setWriter(final Writer writer) {
>>
>> http://git-wip-us.apache.org/repos/asf/logging-log4j2/blob/b
>> 73c589c/log4j-core/src/main/java/org/apache/logging/log4j/co
>> re/appender/rolling/RollingFileManager.java
>> ----------------------------------------------------------------------
>> diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/app
>> ender/rolling/RollingFileManager.java b/log4j-core/src/main/java/org
>> /apache/logging/log4j/core/appender/rolling/RollingFileManager.java
>> index 3f8ba2a..e00319d 100644
>> --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/app
>> ender/rolling/RollingFileManager.java
>> +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/app
>> ender/rolling/RollingFileManager.java
>> @@ -242,7 +242,7 @@ public class RollingFileManager extends FileManager {
>> final RolloverDescription descriptor =
>> strategy.rollover(this);
>> if (descriptor != null) {
>> writeFooter();
>> - close();
>> + closeOutputStream();
>> if (descriptor.getSynchronous() != null) {
>> LOGGER.debug("RollingFileManager executing
>> synchronous {}", descriptor.getSynchronous());
>> try {
>>
>> http://git-wip-us.apache.org/repos/asf/logging-log4j2/blob/b
>> 73c589c/log4j-core/src/main/java/org/apache/logging/log4j/co
>> re/appender/rolling/RollingRandomAccessFileManager.java
>> ----------------------------------------------------------------------
>> diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/app
>> ender/rolling/RollingRandomAccessFileManager.java
>> b/log4j-core/src/main/java/org/apache/logging/log4j/core/app
>> ender/rolling/RollingRandomAccessFileManager.java
>> index 8b39209..f27d445 100644
>> --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/app
>> ender/rolling/RollingRandomAccessFileManager.java
>> +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/app
>> ender/rolling/RollingRandomAccessFileManager.java
>> @@ -128,7 +128,7 @@ public class RollingRandomAccessFileManager extends
>> RollingFileManager {
>> }
>>
>> @Override
>> - public synchronized void close() {
>> + public synchronized void closeOutputStream() {
>> flush();
>> try {
>> randomAccessFile.close();
>>
>> http://git-wip-us.apache.org/repos/asf/logging-log4j2/blob/b
>> 73c589c/log4j-core/src/main/java/org/apache/logging/log4j/co
>> re/net/TcpSocketManager.java
>> ----------------------------------------------------------------------
>> diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/net/TcpSocketManager.java
>> b/log4j-core/src/main/java/org/apache/logging/log4j/core/net
>> /TcpSocketManager.java
>> index 4d83d97..a34ce30 100644
>> --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/net
>> /TcpSocketManager.java
>> +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/net
>> /TcpSocketManager.java
>> @@ -147,8 +147,8 @@ public class TcpSocketManager extends
>> AbstractSocketManager {
>> }
>>
>> @Override
>> - protected synchronized void close() {
>> - super.close();
>> + protected synchronized void closeOutputStream() {
>> + super.closeOutputStream();
>> if (connector != null) {
>> connector.shutdown();
>> connector.interrupt();
>>
>>
>
--
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