[GitHub] [bookkeeper] dlg99 commented on a diff in pull request #3846: Streamline batch add request

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

dlg99 commented on code in PR #3846:
URL: https://github.com/apache/bookkeeper/pull/3846#discussion_r1142769155


##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Journal.java:
##########
@@ -885,6 +887,26 @@ public void logAddEntry(ByteBuf entry, boolean ackBeforeSync, WriteCallback cb,
         logAddEntry(ledgerId, entryId, entry, ackBeforeSync, cb, ctx);
     }
 
+    public void logAddEntries(List<ByteBuf> entries, boolean ackBeforeSync, WriteCallback cb, Object ctx)
+        throws InterruptedException {
+        long reserveMemory = 0;
+        QueueEntry[] queueEntries = new QueueEntry[entries.size()];

Review Comment:
   can we pool these arrays?



##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieRequestProcessor.java:
##########
@@ -205,21 +206,21 @@ public BookieRequestProcessor(ServerConfiguration serverCfg, Bookie bookie, Stat
         readsSemaphore = maxReads > 0 ? new Semaphore(maxReads, true) : null;
     }
 
-    protected void onAddRequestStart(Channel channel) {
+    protected void onAddRequestStart(Channel channel, int permits) {
         if (addsSemaphore != null) {
-            if (!addsSemaphore.tryAcquire()) {
+            if (!addsSemaphore.tryAcquire(permits)) {

Review Comment:
   can try something like 
   ```
   int minPermits = 1;
   int available = addsSemaphore.availablePermits();
   permits = Math.min(Math.max(minPermits, available), permits)
   ```
   to reduce wait for permits. make onAddRequestStart return actual number of permits acquired, go from there.
   
   The risk is to drop to 1 almost all the time under load, but otherwise the load will be very spiky (switching from nothing to batch, creating more spiky load on the disk).
   This can be helped with permits > minPermits > 1
   
   have you tested with backpressure/maxAddsInProgressLimit enabled?
   



##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieRequestProcessor.java:
##########
@@ -205,21 +206,21 @@ public BookieRequestProcessor(ServerConfiguration serverCfg, Bookie bookie, Stat
         readsSemaphore = maxReads > 0 ? new Semaphore(maxReads, true) : null;
     }
 
-    protected void onAddRequestStart(Channel channel) {
+    protected void onAddRequestStart(Channel channel, int permits) {
         if (addsSemaphore != null) {
-            if (!addsSemaphore.tryAcquire()) {
+            if (!addsSemaphore.tryAcquire(permits)) {
                 final long throttlingStartTimeNanos = MathUtils.nowInNano();
                 channel.config().setAutoRead(false);
                 LOG.info("Too many add requests in progress, disabling autoread on channel {}", channel);
                 requestStats.blockAddRequest();
-                addsSemaphore.acquireUninterruptibly();
+                addsSemaphore.acquireUninterruptibly(permits);

Review Comment:
   permits must be <= maxAdds or this will never succeed.
   



-- 
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