On item 3, the main benefit of an ExecutorService is that the threads are managed and don’t have to be created for each and every task. If the task runs repeatedly, as might happen on a busy system with a max file size and lots of logging going on, this can be beneficial. But when something runs very infrequently and shutting down the ExecutorService is more trouble than it would be to just create the thread and start it, then it obviously isn’t worth it. I believe that is the case with handling reconfiguration, but running the RollingFileManager’s async tasks could go either way.
Ralph > On Jan 14, 2017, at 7:41 PM, Remko Popma <remko.po...@gmail.com> wrote: > > Item 1 - this is not a big deal to me, I just noticed it. I can understand > Mikael's reasoning that keeping this service would encourage contributors to > schedule tasks on non-daemon threads and we need to be judicious about that. > Item 2 - ConfigurationScheduler's design, which avoids starting the > background thread if not necessary, makes sense for its use case but also > means it may not be easily repurposed as the general thread pool for all > async tasks. > Item 3 - I was leaning toward just creating ad hoc (non-daemon) threads for > rollover async compress tasks. That ensures parallel execution. I suppose > having an ExecutionService with some reasonable number of max threads would > also achieve this. But what would be the benefit of having an > ExecutionService instead of ad hoc threads? > Item 4 - I just fixed this (see LOG4J2-1786 > <https://issues.apache.org/jira/browse/LOG4J2-1786>). > > > On Sun, Jan 15, 2017 at 11:26 AM, Apache <ralph.go...@dslextreme.com > <mailto:ralph.go...@dslextreme.com>> wrote: > Item 1 - It should not be removed. > Item 2 - The RollingFileAppender could increment the scheduled items if an > instance is configured. Ideally, it should only increment it once. > Item 3 - It might make more sense for the RollingFileManager to use a > ThreadPoolExecutorService that it owns for the async task. > Item 4 - I will have to look into this one. > > Ralph > >> On Jan 12, 2017, at 9:01 AM, Remko Popma <remko.po...@gmail.com >> <mailto: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 >> <mailto: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 >> <http://git-wip-us.apache.org/repos/asf/logging-log4j2/repo> >> Commit: >> http://git-wip-us.apache.org/repos/asf/logging-log4j2/commit/162a5e33 >> <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 >> <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 >> <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 >> <mailto:mikael.stal...@magine.com>> >> Authored: Thu Jan 12 12:14:18 2017 +0100 >> Committer: Mikael Ståldal <mikael.stal...@magine.com >> <mailto:mikael.stal...@magine.com>> >> Committed: Thu Jan 12 12:32:00 2017 +0100 >> >> ---------------------------------------------------------------------- >> .../logging/log4j/core/LoggerContext.java | 97 +------------------- >> .../core/appender/mom/kafka/KafkaManager.java | 11 ++- >> .../appender/rolling/RollingFileManager.java | 2 +- >> .../core/config/AbstractConfiguration.java | 4 +- >> .../core/config/ConfigurationScheduler.java | 51 +++++++--- >> .../core/config/ConfiguratonFileWatcher.java | 7 +- >> .../logging/log4j/core/config/Configurator.java | 4 +- >> .../log4j/core/util/Log4jThreadFactory.java | 11 --- >> .../nosql/appender/cassandra/CassandraRule.java | 2 +- >> src/site/xdoc/manual/configuration.xml.vm | 4 +- >> 10 files changed, 59 insertions(+), 134 deletions(-) >> ---------------------------------------------------------------------- >> > >
