[GitHub] ivankelly commented on a change in pull request #1225: Issue #570: getting rid of unnecessary synchronization in InterleavedLedgerStorage

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

ivankelly commented on a change in pull request #1225: Issue #570: getting rid of unnecessary synchronization in InterleavedLedgerStorage
URL: https://github.com/apache/bookkeeper/pull/1225#discussion_r172130455
 
 

 ##########
 File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/InterleavedLedgerStorage.java
 ##########
 @@ -360,10 +362,10 @@ public void checkpoint(Checkpoint checkpoint) throws IOException {
 
     @Override
     public synchronized void flush() throws IOException {
-        if (!somethingWritten) {
+        if (!somethingWritten.get()) {
 
 Review comment:
   with the compareAndSet, the only thing protected here is the flushOrCheckpoint(), which is called unprotected from checkpoint() above, so I think it would be safe. 
   
   It only seems to be called from the syncthread, which means it's effectively synchronized anyhow since it's all on a single thread, and the synchronized here was only to protect the somethingWritten flag. 
   
   It's no harm to leave it in, just seems weird to be mixing atomics and synchronization without good reason.

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