> > Not an issue with master as blockAddCompletions has been replaced with > > a simple boolean and failure handling changed to only deal with one > > failure at a time. However, looking at it again, I think I did spot a > > similar bug. will dig in after I send this. > > Great; looking forward.
It was something that sam had raised in the original patch. Fix is up. https://github.com/apache/bookkeeper/pull/1857 > updateMetadataIfPossible() is called from resolveConflict() -> "return > updateMetadataIfPossible(newMeta);" > There is only one case in the updateMetadataIfPossible(newMeta); that will > rereadMetadata in other cases > it can just return TRUE so there is no ChangeEnsembleCb. When updateMetadataIfPossible is called, the precondition is that there is at least one 'bump' that needs to be decremented on blockAddCompletions before adds can be completed. There's 3 branches in the code. 1. existing metadata is newer. reread is called, which will eventually call updateMetadataIfPossible again under the same conditions 2. isConflictWith is true, so kill the ledger, we don't want to decrement because we are in an undefined condition 3. we write new metadata, in which case ChangeEnsembleCb does the decrement. This code is awful. It gives me nightmares. But AFAICS it is correct w.r.t. to the preconditions stated above. [1] https://github.com/apache/bookkeeper/blob/621799172a0de84f97b87d795c31e263de80e8a3/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerHandle.java#L2197 -Ivan
