[GitHub] [bookkeeper] sijie commented on a change in pull request #2150: Improve journal throughput when journalSyncData is disabled.

[GitBox <gi...@apache.org>]

sijie commented on a change in pull request #2150: Improve journal throughput when journalSyncData is disabled.
URL: https://github.com/apache/bookkeeper/pull/2150#discussion_r317185790
 
 

 ##########
 File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Journal.java
 ##########
 @@ -1050,25 +1050,11 @@ public void run() {
                                 .registerSuccessfulValue(batchSize);
 
                             boolean shouldRolloverJournal = (lastFlushPosition > maxJournalSize);
-                            if (syncData) {
 
 Review comment:
   > is it expected ?
   
   yes. that's expected. The flag `journalSyncData` was meant for any write do you need to wait for syncing journal data before triggering the callback.
   
   > I think we should cover this change with some test, we already have some kind of advanced tests on journal flush, I added them while working on DEFERRED_SYNC.
   
   I don't think we need any new tests. because existing tests already covered both cases. What we need to adjust here is to make sure those tests are aligned with the behavior fixed in this pull request.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services