gemmellr commented on a change in pull request #44:
URL: https://github.com/apache/qpid-jms/pull/44#discussion_r745494450
##########
File path:
qpid-jms-client/src/main/java/org/apache/qpid/jms/JmsConnectionFactory.java
##########
@@ -368,6 +377,19 @@ protected static URI createURI(String name) {
return null;
}
+ protected Supplier<Holder<ExecutorService>>
getCompletionExecutorServiceFactory() {
+ if (this.completionThreads == 0) {
+ return null;
+ }
+ synchronized (this) {
+ if (completionExecutorServiceFactory == null) {
+ QpidJMSForkJoinWorkerThreadFactory fjThreadFactory = new
QpidJMSForkJoinWorkerThreadFactory("completion thread pool", true);
+ completionExecutorServiceFactory = sharedRefCnt(() -> new
ForkJoinPool(completionThreads, fjThreadFactory, null, false),
ThreadPoolUtils::shutdown);
Review comment:
I meant was it an attempt to avoid creating the pool until the
completion executor is used. Not 'avoid creating it' in terms of pre-allocating
it with the factory.
This bit is only used after the point createConnection has been called, and
only if the option was set. I think its reasonable to assume the pool is to be
used from then given its an explicitly set option, and I dont see anyone ever
setting it unless they want the behaviour.
Given that, the mechanism still all seems rather overcomplicated. This feels
like a relatively simple case, an 'if there is an existing pool, then use that,
otherwise create one' check coupled with the opposing cleanup. One that should
be relatively infrequently used. It seems like even a simple synchronized block
with a count inside could do?
I'm also weighing this vast mechanism against against a change that enables
supplying a pool via the factory extension mechanism, which would probably be
something ridiculous like 5-10 lines in comparison.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
