[GitHub] [bookkeeper] horizonzy commented on a diff in pull request #3843: Use BatchedArrayBlockingQueue in Journal

["horizonzy (via GitHub)" <gi...@apache.org>]

horizonzy commented on code in PR #3843:
URL: https://github.com/apache/bookkeeper/pull/3843#discussion_r1142881335


##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Journal.java:
##########
@@ -472,35 +472,30 @@ public void run() {
                 }
             }
 
-            final List<ForceWriteRequest> localRequests = new ArrayList<>();
+            final ForceWriteRequest[] localRequests = new ForceWriteRequest[conf.getJournalQueueSize()];
 
             while (running) {
                 try {
-                    int numReqInLastForceWrite = 0;
+                    int numEntriesInLastForceWrite = 0;
 
-                    int requestsCount = forceWriteRequests.drainTo(localRequests);
-                    if (requestsCount == 0) {
-                        ForceWriteRequest fwr = forceWriteRequests.take();
-                        localRequests.add(fwr);
-                        requestsCount = 1;
-                    }
+                    int requestsCount = forceWriteRequests.takeAll(localRequests);
 
                     journalStats.getForceWriteQueueSize().addCount(-requestsCount);
 
                     // Sync and mark the journal up to the position of the last entry in the batch
-                    ForceWriteRequest lastRequest = localRequests.get(requestsCount - 1);
+                    ForceWriteRequest lastRequest = localRequests[requestsCount - 1];
                     syncJournal(lastRequest);
 
                     // All the requests in the batch are now fully-synced. We can trigger sending the
                     // responses
                     for (int i = 0; i < requestsCount; i++) {
-                        ForceWriteRequest req = localRequests.get(i);
-                        numReqInLastForceWrite += req.process();
+                        ForceWriteRequest req = localRequests[i];
+                        numEntriesInLastForceWrite += req.process();
                         req.recycle();
                     }
-
+                    Arrays.fill(localRequests, 0, requestsCount, null);

Review Comment:
   > Why do we need to fill the array with null? It will overwrite by the next, no?
   
   At the before time, we read the 100 elements from the queue. the localRequests array elements will be filled from 0-100.
   But the next time, if the queue only has 50 elements, the the localRequests array 0-50 will be overridden, the 51-100 elements still be the before-time elements.
   
   The reference is strong, it's bad for GC.



-- 
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: commits-unsubscribe@bookkeeper.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org