1 - OK, keeping it but adding a comment with a warning. 2 - Leaving as is.
3 - Using an ad-hoc non-daemon thread for rollover tasks. Using a non-daemon ad-hoc should be OK since it will die immediately after the task is finished. But we should not have pools with non-daemon threads since they will keep idle non-daemon threads around. (This is basically a revert to how it used to work in 2.6.) 4 - Merged fix from master. Please review branch again. On Jan 15, 2017 4:00 AM, "Apache" <ralph.go...@dslextreme.com> wrote: > 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> > 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> 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/1 >>> 62a5e33 >>> Diff: http://git-wip-us.apache.org/repos/asf/logging-log4j2/diff/1 >>> 62a5e33 >>> >>> Branch: refs/heads/LOG4J2-1748and1780-remove-ExecutorService-from-Lo >>> ggerContext >>> 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 >>> >>> ---------------------------------------------------------------------- >>> .../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(-) >>> ---------------------------------------------------------------------- >>> >>> >> > >
