logging-log4j-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Remko Popma <remko.po...@gmail.com>
Subject Re: logging-log4j2 git commit: LOG4J2-1748 and LOG4J2-1780 Remove ExecutorServices from LoggerContext
Date Fri, 13 Jan 2017 11:37:22 GMT
Can you also answer the points I raised in my feedback?

Remko
Sent from my iPhone

> On Jan 13, 2017, at 20:30, Mikael Ståldal <mikael.staldal@magine.com> wrote:
> 
> I still think that we should not use non-daemon threads (except in tests and samples).
> 
>> On Jan 12, 2017 11:45 PM, "Gary Gregory" <garydgregory@gmail.com> wrote:
>> I still like the work I did with executor services, so I'll leave it at that ;-)
Sure it turns out there is a bug on reconfigure but it's fixable without throwing it all out.
I'm swamped at work ATM so I cannot spend time dealing with too much FOSS this week (as was
last week). Good luck :-)
>> 
>> Gary
>> 
>>> On Thu, Jan 12, 2017 at 8:01 AM, Remko Popma <remko.popma@gmail.com> wrote:
>>> Some feedback:
>>> 1. Why remove Log4jThreadFactory.createThreadFactory?
>>> CassandraRule was using it and now needs to use the constructor.
>>> 
>>> 2. The ConfigurationScheduler may create a thread pool with size zero.  ConfigurationScheduler::incrementScheduledItems
is only called if monitorInterval is positive, or if a plugin with the @Scheduled annotation
is configured (not sure I'm reading that right), or if a CronTriggeringPolicy is configured.
If none of these are true, but a RollingFile appender is configured, I think there will be
a problem. 
>>> 
>>> 3. Generally, I think RollingFileManager AsyncActions (compress) should not be
submitted to the ConfigurationScheduler: this executor will usually only have one thread so
these actions will execute one by one sequentially. If many files need to be rolled over and
compressed at the same time, I think this work should be done in parallel as much as possible.Therefore,
I think it's better to create new (non-daemon) threads for the rollover async actions.
>>> 
>>> 4. (Not your change, I noticed during review): ConfigurationScheduler catches
InterruptedException but does not restore the interrupted flag. Should be:
>>> ...
>>> } catch (InterruptedException ie) {
>>> ...
>>>     // Preserve interrupt status
>>>     Thread.currentThread().interrupt();
>>> }
>>> 
>>> 
>>>> On Thu, Jan 12, 2017 at 8:36 PM, <mikes@apache.org> wrote:
>>>> Repository: logging-log4j2
>>>> Updated Branches:
>>>>   refs/heads/LOG4J2-1748and1780-remove-ExecutorService-from-LoggerContext
[created] 162a5e33a
>>>> 
>>>> 
>>>> LOG4J2-1748 and LOG4J2-1780 Remove ExecutorServices from LoggerContext
>>>> 
>>>> 
>>>> Project: http://git-wip-us.apache.org/repos/asf/logging-log4j2/repo
>>>> Commit: http://git-wip-us.apache.org/repos/asf/logging-log4j2/commit/162a5e33
>>>> Tree: http://git-wip-us.apache.org/repos/asf/logging-log4j2/tree/162a5e33
>>>> Diff: http://git-wip-us.apache.org/repos/asf/logging-log4j2/diff/162a5e33
>>>> 
>>>> Branch: refs/heads/LOG4J2-1748and1780-remove-ExecutorService-from-LoggerContext
>>>> Commit: 162a5e33ace3f1b4adf0726502083d0efb1e799a
>>>> Parents: a6f9d12
>>>> Author: Mikael Ståldal <mikael.staldal@magine.com>
>>>> Authored: Thu Jan 12 12:14:18 2017 +0100
>>>> Committer: Mikael Ståldal <mikael.staldal@magine.com>
>>>> Committed: Thu Jan 12 12:32:00 2017 +0100
>>>> 
>>>> ----------------------------------------------------------------------
>>>>  .../
>> 
>> 

Mime
View raw message