Allow to send callbacks from journal thread #1258
Conversation
| this.cbThreadPool = Executors.newFixedThreadPool(conf.getNumJournalCallbackThreads(), | ||
| new DefaultThreadFactory("bookie-journal-callback")); | ||
| } else { | ||
| this.cbThreadPool = MoreExecutors.newDirectExecutorService(); |
There was a problem hiding this comment.
this thread pool gets used on two different threads(JournalThread & ForceWriteThread). Are you sure this is safe?
There was a problem hiding this comment.
I think it would be anyway either one case or the other.
I think though that it should be safe anyway. If you have a "direct" executor, it is like a dummy executor.
We could have threads t1 and t2 sharing the same instance. when t1 submits one task, it gets executed immediately in t1. t2 tasks will be executed in t2 thread.
There was a problem hiding this comment.
Previously I think it wasn't possible. if getNumJournalCallbackThreads was 0, then we would throw an exception creating the executor. If 1, then callbacks would be serialized. That said, if you had getNumJournalCallbackThreads > 1, you have no guarantee where you run, they would have to be thread safe anyhow.
| this.cbThreadPool = Executors.newFixedThreadPool(conf.getNumJournalCallbackThreads(), | ||
| new DaemonThreadFactory()); | ||
| if (conf.getNumJournalCallbackThreads() > 0) { | ||
| this.cbThreadPool = Executors.newFixedThreadPool(conf.getNumJournalCallbackThreads(), |
There was a problem hiding this comment.
interesting.. I thought we used OrderedScheduler here as well.
I actually checked twitter's branch, it does use ordered scheduler. I believe cbThreadPool was first introduced by twitter sometime back.
There was a problem hiding this comment.
the change looks good to me though. I just found this difference here.
|
retest this please. |
In BK 4.3 it was possible to configure 0 threads for the dispatching of journal callback threads.
There might be good reasons to not use a thread pool when dispatching the callback threads:
We should allow the same convention (threads=0 means direct execution) that we have in other places.