Code review comment for lp:~pbeaman/akiban-persistit/fix-accumulator-checkpoint-failure

Revision history for this message
Peter Beaman (pbeaman) wrote :

I think my explanation of the updateBaseValue call is a little off. I'll fix the Javadoc. updateBaseValue is called from RecoveryManager.DefaultRecoveryListener to apply a change from a committed transaction that has not yet been checkpointed. So in fact the setting of _checkpointRef in checkpointNeeded will occur precisely when a checkpoint is in fact needed, and there should be no redundant checkpointing. Re 0 vs. the commit timestamp of the transaction: the choice is immaterial; the intent is to set _checkpointRef so that the first checkpoint cycle of the new epoch will checkpoint the accumulator. If updateBaseValue were engaged in a race with the checkpoint manager then the distinction would matter, but checkpoint manager starts after recovery is done. But upon reflection, given that someday someone might change that, I'll change it to pass commit timestamp.

Re return vs. break at +47: I think that might be okay, but it complicates the "proof." Need to think about that one. As is, I believe the code is correct, but the change could eliminate a redundant set operation on volatile _checkpointRef and that might be a tiny help with highly concurrent transactions.

« Back to merge proposal