A fair amount of this code is duplicated from the existing markSeqNo method. Since the first check is simply a bounds check, we can write a shouldUpdateSeqNo method that does this and returns a boolean to decide if further processing is necessary. This can be invoked by wrapper methods that perform the update by compareAndSet (as below) or via updateCheckpoint (as with the original method)

Shouldn't this be using updateCheckpoint to ensure that the persistedSeqNo is kept up to date?

The call to updateCheckpoint to ensure persistedSeqNo is up to date still takes place when markSeqNoAsPersisted is called. In the case of segrep, we update processedSeqNo just once at the end since indexing doesn't take place on the replica - we omit the call to updateCheckpoint since it checks that all seq numbers till that point are consecutively processed.

Thanks for the explanation! I think we should capture this bit in the Javadoc for fastForwardProcessedSeqNo:

In the case of segrep, we update processedSeqNo just once at the end since indexing doesn't take place on the replica - we omit the call to updateCheckpoint since it checks that all seq numbers till that point are consecutively processed.

These should really be 3 separate tests since you're testing different use-cases

1. Base case
2. Idempotency
3. Persisted-vs-processed being out-of-sync, checkpoint update for seg-rep, and finally idempotency again

A suggestion here, can we name this after what it's doing (fast forwarding to seqNo) vs the replication strategy that invokes it?

I don't think we need this segrep specific logic and it can be inside of fastForwardProcessedSeqNo leaving this method as is. This class shouldn't care about segrep vs non segrep.

It also looks like waitForProcessedOpsToComplete is called from tests only, I'm not sure we have a use case for it with this call path? If not then we won't need to wait/notify.

Removed the wait/notify and separated out the segrep specific logic

I think you can put the check against the persisted checkpoint inside of fastForwardProcessedSeqNo and avoid the segrep specific flag.

Changed the logic in the check to avoid segrep specific flag

Nit - Since there's no check here for seg-rep, the wording of "if segment replication is enabled" is confusing. Rephrase to:

Updates the processed sequence checkpoint to the given value. This does not update the persisted checkpoint value. This method is only used for segment replication.

I realize that this was done in response to my comment, but this may be too far in the direction of unification 😄

What do you think of the following structure?

1. Instead of branching on this boolean value, have this method return the boolean from shouldUpdateSeqNo (or in-line that method's logic here).
2. Have a single method that executes the else part of this - both markSeqNoAsPersisted and markSeqNoAsProcessed can invoke this method. This will cover the existing code flows.
3. Finally, move the if clause portion directly into fastForwardProcessedSeqNo - this will cover the seg-rep code flow.

shouldUpdateSeqNo doesn't have the logic to return a boolean representing segrep, but followed the other suggestions to have separate code flows

As @mch2 alluded to in an earlier comment - if you're having to write variables to model use-cases, then that's usually a sign of an overfit. Consider having this method signature be:

private boolean shouldUpdateSeqNo(final long seqNo, final AtomicLong lowerBound, @Nullable final AtomicLong upperBound)

Also see my other comment.

Followed the signature suggested

Thanks for the explanation! I think we should capture this bit in the Javadoc for fastForwardProcessedSeqNo:

In the case of segrep, we update processedSeqNo just once at the end since indexing doesn't take place on the replica - we omit the call to updateCheckpoint since it checks that all seq numbers till that point are consecutively processed.