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.stal...@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" <garydgreg...@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.po...@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, <mi...@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.stal...@magine.com>
>>>> Authored: Thu Jan 12 12:14:18 2017 +0100
>>>> Committer: Mikael Ståldal <mikael.stal...@magine.com>
>>>> Committed: Thu Jan 12 12:32:00 2017 +0100
>>>>
>>>> ----------------------------------------------------------------------
>>>> .../
>>
>>
