fix local processed checkpoint update #2576

Poojita-Raj · 2022-03-23T21:06:26Z

Signed-off-by: Poojita Raj [email protected]

Description

For segment replication, previously the local processed checkpoint was not being updated. This is now fixed along with unit tests for the same.

Issues Resolved

Resolves #2358

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
- New functionality has javadoc added
Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

opensearch-ci-bot · 2022-03-23T21:31:46Z

❌ Gradle Check failure e1ff2b4c54a5e6a9b8d1e93424bf3ccaf02175bc
Log 3717

Reports 3717

opensearch-ci-bot · 2022-03-23T21:32:03Z

❌ Gradle Check failure a6c66306313d64689f03b7168a7f2a345756af90
Log 3718

Reports 3718

opensearch-ci-bot · 2022-03-23T21:32:20Z

❌ Gradle Check failure 52bf531bb130251fd3caf79e75ffc6cd369c292a
Log 3719

Reports 3719

opensearch-ci-bot · 2022-03-23T22:07:33Z

❌ Gradle Check failure 6251b8effc07c20369d1d87f14a12db45471d1b7
Log 3721

Reports 3721

kartg · 2022-03-24T00:24:26Z

server/src/main/java/org/opensearch/index/seqno/LocalCheckpointTracker.java

+        assert Thread.holdsLock(this);
+        advanceMaxSeqNo(seqNo);
+        if ((seqNo > persistedCheckpoint.get()) || (seqNo <= processedCheckpoint.get())) {
+            return;
+        }


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)

kartg · 2022-03-24T00:25:58Z

server/src/main/java/org/opensearch/index/seqno/LocalCheckpointTracker.java

+            return;
+        }
+        try {
+            processedCheckpoint.compareAndSet(processedCheckpoint.get(), seqNo);


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.

kartg · 2022-03-24T00:31:53Z

.../src/test/java/org/opensearch/index/seqno/SegmentReplicationLocalCheckpointTrackerTests.java

+        tracker = createEmptyTracker();
+    }
+
+    public void testSimpleSegrepPrimaryProcessed() {


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

Base case

Idempotency

Persisted-vs-processed being out-of-sync, checkpoint update for seg-rep, and finally idempotency again

mch2 · 2022-03-24T17:58:53Z

server/src/main/java/org/opensearch/index/seqno/LocalCheckpointTracker.java

+     *
+     * @param seqNo the sequence number to mark as processed
+     */
+    public synchronized void segrepMarkSeqNoAsProcessed(final long seqNo) {


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

opensearch-ci-bot · 2022-03-24T23:18:21Z

❌ Gradle Check failure 0b1dc1ddfba991abb32bbded801fa125b12561c2
Log 3744

Reports 3744

opensearch-ci-bot · 2022-03-24T23:37:02Z

❌ Gradle Check failure a428afd8d713c7ac2e86c62f475282cb1a71cbb2
Log 3745

Reports 3745

mch2 · 2022-03-25T18:17:24Z

server/src/main/java/org/opensearch/index/seqno/LocalCheckpointTracker.java

-        bitSet.set(offset);
-        if (seqNo == checkPoint.get() + 1) {
-            updateCheckpoint(checkPoint, bitSetMap);
+        if (segrep) {


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

mch2 · 2022-03-25T18:23:00Z

server/src/main/java/org/opensearch/index/seqno/LocalCheckpointTracker.java

+    }
+
+    private boolean shouldUpdateSeqNo(final long seqNo, boolean segrep, final AtomicLong checkpoint) {
+        return !((seqNo <= checkpoint.get()) || (segrep && seqNo > persistedCheckpoint.get()));


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

kartg · 2022-03-25T17:24:37Z

server/src/main/java/org/opensearch/index/seqno/LocalCheckpointTracker.java

    }

-    private void markSeqNo(final long seqNo, final AtomicLong checkPoint, final LongObjectHashMap<CountedBitSet> bitSetMap) {
+    /**
+     * Updates the processed checkpoint to the provided sequence number if segment replication is enabled


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.

kartg · 2022-03-25T18:04:56Z

server/src/main/java/org/opensearch/index/seqno/LocalCheckpointTracker.java

-        bitSet.set(offset);
-        if (seqNo == checkPoint.get() + 1) {
-            updateCheckpoint(checkPoint, bitSetMap);
+        if (segrep) {


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?

Instead of branching on this boolean value, have this method return the boolean from shouldUpdateSeqNo (or in-line that method's logic here).

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.

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

kartg · 2022-03-25T18:09:51Z

server/src/main/java/org/opensearch/index/seqno/LocalCheckpointTracker.java

+        markSeqNo(seqNo, processedCheckpoint, null, true);
+    }
+
+    private boolean shouldUpdateSeqNo(final long seqNo, boolean segrep, final AtomicLong checkpoint) {


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

kartg · 2022-03-25T18:22:33Z

server/src/main/java/org/opensearch/index/seqno/LocalCheckpointTracker.java

+            return;
+        }
+        try {
+            processedCheckpoint.compareAndSet(processedCheckpoint.get(), seqNo);


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.

Signed-off-by: Poojita Raj <[email protected]>

opensearch-ci-bot · 2022-03-31T03:19:09Z

❌ Gradle Check failure c35bac2
Log 3954

Reports 3954

* fix local processed checkpoint update Signed-off-by: Poojita Raj <[email protected]> * separated tests + wrapper function Signed-off-by: Poojita Raj <[email protected]>

…t replication] (#2576) (#2883) * fix local processed checkpoint update (#2576) Signed-off-by: Poojita Raj <[email protected]> * separated tests + wrapper function Signed-off-by: Poojita Raj <[email protected]> * moved tests + compareAndSet change Signed-off-by: Poojita Raj <[email protected]>

Poojita-Raj requested a review from a team as a code owner March 23, 2022 21:06

Poojita-Raj force-pushed the feature/segment-replication branch 2 times, most recently from a6c6630 to 52bf531 Compare March 23, 2022 21:15

Poojita-Raj requested review from kartg and mch2 March 23, 2022 21:20

Poojita-Raj force-pushed the feature/segment-replication branch from 52bf531 to 6251b8e Compare March 23, 2022 21:43

kartg reviewed Mar 24, 2022

View reviewed changes

mch2 reviewed Mar 24, 2022

View reviewed changes

Poojita-Raj force-pushed the feature/segment-replication branch from 0b1dc1d to a428afd Compare March 24, 2022 23:21

Poojita-Raj requested review from mch2 and kartg March 24, 2022 23:49

mch2 reviewed Mar 25, 2022

View reviewed changes

kartg reviewed Mar 25, 2022

View reviewed changes

mch2 mentioned this pull request Mar 28, 2022

Fix Segment Replication integ tests #2637

Merged

5 tasks

Poojita-Raj added 2 commits March 30, 2022 13:59

fix local processed checkpoint update

a432fb8

Signed-off-by: Poojita Raj <[email protected]>

separated tests + wrapper function

c35bac2

Signed-off-by: Poojita Raj <[email protected]>

Poojita-Raj force-pushed the feature/segment-replication branch from a428afd to c35bac2 Compare March 31, 2022 03:02

kartg approved these changes Apr 1, 2022

View reviewed changes

Poojita-Raj merged commit 997ef5a into opensearch-project:feature/segment-replication Apr 1, 2022

Poojita-Raj mentioned this pull request Apr 12, 2022

[Segment Replication] Merge back to main tracking issue. #2355

Closed

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix local processed checkpoint update #2576

fix local processed checkpoint update #2576

Poojita-Raj commented Mar 23, 2022

opensearch-ci-bot commented Mar 23, 2022

opensearch-ci-bot commented Mar 23, 2022

opensearch-ci-bot commented Mar 23, 2022

opensearch-ci-bot commented Mar 23, 2022

kartg Mar 24, 2022

kartg Mar 24, 2022

Poojita-Raj Mar 24, 2022

kartg Mar 25, 2022

kartg Mar 24, 2022

mch2 Mar 24, 2022

opensearch-ci-bot commented Mar 24, 2022

opensearch-ci-bot commented Mar 24, 2022

mch2 Mar 25, 2022

Poojita-Raj Mar 31, 2022

mch2 Mar 25, 2022

Poojita-Raj Mar 31, 2022

kartg Mar 25, 2022

kartg Mar 25, 2022

Poojita-Raj Mar 31, 2022

kartg Mar 25, 2022

Poojita-Raj Mar 31, 2022

kartg Mar 25, 2022

opensearch-ci-bot commented Mar 31, 2022

fix local processed checkpoint update #2576

fix local processed checkpoint update #2576

Conversation

Poojita-Raj commented Mar 23, 2022

Description

Issues Resolved

Check List

opensearch-ci-bot commented Mar 23, 2022

opensearch-ci-bot commented Mar 23, 2022

opensearch-ci-bot commented Mar 23, 2022

opensearch-ci-bot commented Mar 23, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

opensearch-ci-bot commented Mar 24, 2022

opensearch-ci-bot commented Mar 24, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

opensearch-ci-bot commented Mar 31, 2022