[GitHub] eolivelli commented on a change in pull request #1436: BP-14 force() API - client side implementation

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

eolivelli commented on a change in pull request #1436: BP-14 force() API - client side implementation
URL: https://github.com/apache/bookkeeper/pull/1436#discussion_r193307572
 
 

 ##########
 File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerHandle.java
 ##########
 @@ -1100,6 +1100,68 @@ public void asyncAddEntry(final long entryId, final byte[] data, final int offse
         cb.addCompleteWithLatency(BKException.Code.IllegalOpException, LedgerHandle.this, entryId, 0, ctx);
     }
 
+    /**
+     * {@inheritDoc}
+     */
+    @Override
+    public CompletableFuture<Void> force() {
+        CompletableFuture<Void> result = new CompletableFuture<>();
+        ForceLedgerOp op = new ForceLedgerOp(this, result);
+        boolean wasClosed = false;
+        synchronized (this) {
+            // synchronized on this to ensure that
+            // the ledger isn't closed between checking and
+            // updating lastAddPushed
+            if (metadata.isClosed()) {
+                wasClosed = true;
+            }
+        }
+
+        if (wasClosed) {
+            // make sure the callback is triggered in main worker pool
+            try {
+                bk.getMainWorkerPool().submit(new SafeRunnable() {
+                    @Override
+                    public void safeRun() {
+                        LOG.warn("Force() attempted on a closed ledger: {}", ledgerId);
+                        result.completeExceptionally(new BKException.BKLedgerClosedException());
 
 Review comment:
   We have discussed about this point. Final answer was to leave this up to the application.
   Filesystems usually do to fsync on close.
   
   I have a test case for this scenario. If you do not perform force() LAC does not advance and metadata are sealed with the current LAC, which could be -1 if no force() has been issued and completed with success. This is consistent with what readers had seen, that is a LAC which never has been updated.
   I see this is not obvious and should be clearly stated in high level docs about BP14, not only in javadocs

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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