zymap commented on code in PR #3544:
URL: https://github.com/apache/bookkeeper/pull/3544#discussion_r998831707
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Journal.java:
##########
@@ -1080,7 +1083,19 @@ journalFormatVersionToWrite, getBufferedChannelBuilder(),
.registerSuccessfulEvent(MathUtils.elapsedNanos(dequeueStartTime),
TimeUnit.NANOSECONDS);
}
- if (numEntriesToFlush == 0) {
+ if (localQueueEntries.isEmpty()) {
+ queue.drainTo(localQueueEntries);
+ }
+
+ if (!localQueueEntries.isEmpty()) {
Review 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](https://github.com/apache/bookkeeper/pull/3544/files#diff-0a79d54e3dfd4bab1d85510ef957f8f4b7ab6f6acd09aadd634b4d7bbee6e1f4R1098)
I have another question. I think this is only to replace the `queue.take();`
operation, so why not put
[them](https://github.com/apache/bookkeeper/pull/3544/files#diff-0a79d54e3dfd4bab1d85510ef957f8f4b7ab6f6acd09aadd634b4d7bbee6e1f4R1086-R1098)
to the line
[1100](https://github.com/apache/bookkeeper/pull/3544/files#diff-0a79d54e3dfd4bab1d85510ef957f8f4b7ab6f6acd09aadd634b4d7bbee6e1f4R1100)?
@merlimat
--
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]
