-
Notifications
You must be signed in to change notification settings - Fork 407
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Create Disruptor with a ThreadFactory instead of ExecutorService #570
Create Disruptor with a ThreadFactory instead of ExecutorService #570
Conversation
After this change, the AsyncDisruptorAppender no longer uses its executorService. The executorService is now only used by the AbstractLogstashTcpSocketAppender. I think we have two options here:
I'm leaning in favor of 1. If you agree, can you update the javadoc of the field, and perhaps the getter as well? We could also deprecate the executorService on AsyncDisruptorAppender with the intention of moving it down to AbstractLogstashTcpSocketAppender later. Thoughts? |
I see your point. However, we are moving to I'd like to start the 7 series with as less deprecation as possible... Removing the So I would go for option (2) + describe the workaround in the release note. |
Yeah, I think 2 is acceptable since we're bumping the major version. |
… the AbstractLogstashTcpSocketAppender AsyncDisruptorAppender does use the ExecutorService anymore - move it inside classes that need it
85128ac
to
ceb78f2
Compare
Ok - done.
(forget about the above - I was wrong - the ScheduledThreadPoolExecutor keeps only coreSize threads in the pool and discard any additional thread immediately after a task is completed and no more is waiting in the queue. Using a coreSize of zero is almost identical to not pooling anything at all) |
@philsttr Are you ok with these changes? Can I squash & merge? |
src/main/java/net/logstash/logback/appender/AbstractLogstashTcpSocketAppender.java
Outdated
Show resolved
Hide resolved
src/test/java/net/logstash/logback/appender/LogstashTcpSocketAppenderTest.java
Show resolved
Hide resolved
src/test/java/net/logstash/logback/appender/LogstashTcpSocketAppenderTest.java
Show resolved
Hide resolved
src/test/java/net/logstash/logback/appender/LogstashTcpSocketAppenderTest.java
Show resolved
Hide resolved
src/main/java/net/logstash/logback/appender/AbstractLogstashTcpSocketAppender.java
Show resolved
Hide resolved
…st one thread in the pool
I need to add some words in the release note about the removal of the ExecutorService from the AsyncDisruptorAppender. Could you please create a first draft of the release note? I haven't done this yet with Github... I'll take care to fill in the blanks afterwards. |
Sure thing. Give me a few mins. |
Np. Time for me to go to sleep anyway ;-) |
See #567