Use BlockingQueue.drainTo() in Journal thread #3544
Conversation
|
rerun the failure tests |
| queue.drainTo(localQueueEntries); | ||
| } | ||
|
|
||
| if (!localQueueEntries.isEmpty()) { |
There was a problem hiding this comment.
If new entries keep put into the queue, the localQueueEntries won't be empty, it won't go into line#1107 branch, and the flush won't be triggered.
There was a problem hiding this comment.
I'm not sure I got the scenario here. I think every time the loop goes back to the top on the while (true) and we should always check first the localQueueEntries before attempting the poll()
There was a problem hiding this comment.
Yes. We should check it before poll. But we shouldn't connect other logic with else if.
When there is no entry put in, the localQueueEntries will be empty. But if there have entries that continue to push, we will never go to other branches.
So the main problem is we never have a chance to go here
I have another question. I think this is only to replace the queue.take(); operation, so why not put them to the line 1100?
@merlimat
There was a problem hiding this comment.
Very good points. I actually think we should do it both before the poll() and the take().
@zymap @hangc0276 PTAL again
Motivation
Use
BlockingQueue.drainTo()in the Journal thread to minimize the contention between this thread and the threads passing entries to the journal.The drain method in the
ArrayBlockingQueuemethod will use the mutex only once and take out all the items in one shot.The logic in the journal dequeuing is already quite complicated. This change aims at not changing the whole logic, rather doing the minimal intervention.