[GitHub] [bookkeeper] eolivelli commented on a diff in pull request #3813: [Bug] Always one orphan ledger is created

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

eolivelli commented on code in PR #3813:
URL: https://github.com/apache/bookkeeper/pull/3813#discussion_r1118406096


##########
stream/distributedlog/core/src/main/java/org/apache/distributedlog/bk/SimpleLedgerAllocator.java:
##########
@@ -382,7 +382,9 @@ private synchronized void completeAllocation(LedgerHandle lh) {
                 allocatePath, DistributedLogConstants.EMPTY_BYTES, (int) version.getLongVersion());
         ZKVersionedSetOp commitOp = new ZKVersionedSetOp(zkSetDataOp, this);
         tryObtainTxn.addOp(commitOp);
-        setPhase(Phase.HANDING_OVER);
+        if (this.phase != Phase.ALLOCATED) {

Review Comment:
   It is common to get here when phase is ALLOCATED
   see here
   
   https://github.com/apache/bookkeeper/blob/e6722848e0a599f215e68e1340196c9dfe8e906e/stream/distributedlog/core/src/main/java/org/apache/distributedlog/bk/SimpleLedgerAllocator.java#L432
   
   or you can get to here from this path
   https://github.com/apache/bookkeeper/blob/e6722848e0a599f215e68e1340196c9dfe8e906e/stream/distributedlog/core/src/main/java/org/apache/distributedlog/bk/SimpleLedgerAllocator.java#L287
   
   maybe it is clearer to not use "!=" but to explicitly enumerate the "phases" in which you are allowed to transition to `HANDING_OVER` .
   
   Could you please change the condition to a "positive" condition ?
   also we could add assertions and fail in case we reach to this point in an unexpected case
   
   if using the phase is not enough to discriminate the reason why to set HANDING_OVER maybe we could change the signature of this method and pass the expected "nextPhase" or something like that



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