Online Indexer: replace the synchronized runner with a heartbeat #3530

jjezra · 2025-08-04T15:30:30Z

Each indexing session, for each index, will create a key-value heartbeat of the format:
[prefix, xid] -> [indexing-type, genesis time, heartbeat time]
Indexing session that are expected to be exclusive will throw an exception if another, active, session exists.

Motivation:
1. Represent the heartbeat in every index during multi target indexing (currently - only the master index has a sync lock)
2. Keep a heartbeat during mutual indexing, which can allow better automatic decision making
3. Decide about exclusiveness according to the indexing method (currently - user input)

Note that with this change, the equivalent of a sync lock will be determined by the indexing type and cannot be set by the users. The index configuration function setUseSynchronizedSession will have no effect on the indexing process.

Resolve #3529

Each indexing session, for each index, will create a key-value heartbeat of the format: [prefix, xid] -> [indexing-type, genesis time, heartbeat time] Indexing session that are expected to be exclusive will throw an exception if another, active, session exists. Motivation: 1. Represent the heartbeat in every index during multi target indexing (currently - only the master index has a sync lock) 2. Keep a heartbeat during mutual indexing, which can allow better automatic decision making 3. Decide about exclusiveness according to the indexing method (currently - user input) Resolve FoundationDB#3529

ScottDugas

I didn't get a chance to look at all of the code, we can discuss some of the things offline.

ScottDugas · 2025-08-12T20:15:18Z

...st/java/com/apple/foundationdb/record/provider/foundationdb/OnlineIndexerBuildIndexTest.java

@@ -180,7 +178,6 @@ <M extends Message> void singleRebuild(
        if (!safeBuild) {


What does safeBuild here mean in this context? Do we need to continue to prevent concurrency?

The things that had left to non-safeBuild, as it seems, are clearing and marking the index as write-only and setting constrains that, IIUIC, should not be happening after the previous clearing.

After so many changes in the code, maybe it is time to re-design (and possibly simplify) the singleRebuild. Should that be a separate PR?

ScottDugas · 2025-08-12T20:16:40Z

...st/java/com/apple/foundationdb/record/provider/foundationdb/OnlineIndexerBuildIndexTest.java

@@ -238,12 +235,6 @@ <M extends Message> void singleRebuild(
                    });
                }
            }
-            if (safeBuild) {


Should this be replaced with checking that no heartbeats exist?

✅

I've added it unconditionally after any build.

ScottDugas · 2025-08-12T20:18:17Z

...ain/java/com/apple/foundationdb/record/provider/foundationdb/OnlineIndexOperationConfig.java

    private final long leaseLengthMillis;

    public static final long UNLIMITED_TIME = 0;

    OnlineIndexOperationConfig(int maxLimit, int initialLimit, int maxRetries, int recordsPerSecond, long progressLogIntervalMillis, int increaseLimitAfter,
                               int maxWriteLimitBytes, long timeLimitMilliseconds, long transactionTimeLimitMilliseconds,
-                               boolean useSynchronizedSession, long leaseLengthMillis) {


Would it make sense to leave the parameter around, and have it inform the heartbeat usage, such that what this actually means is "Do not allow two builds of this index at the same time"? Perhaps, only as a temporary change to reduce breaking changes?

I do not know the reasons for originally allowing the non-synchronized mode. I assume that it was either done to allow concurrent indexing of single ranges - which is irrelevant now.
Mutual indexing had been already ignoring this parameter for practical reasons, and concurrent "no sync lock" indexing methods that are optimized for exclusiveness will now throw a "sync exception" (deterministically on the newest indexer) instead of a vague "conflicts exception" (on a random indexer). FMHO, this is not something worth avoiding.

ScottDugas · 2025-08-12T20:18:34Z

...ain/java/com/apple/foundationdb/record/provider/foundationdb/OnlineIndexOperationConfig.java

@@ -188,10 +186,6 @@ public static Builder newBuilder() {
        return new Builder();
    }

-    public boolean shouldUseSynchronizedSession() {


Should this stick around as a deprecated method?

✅

(I assumed that this function is not really a part of the API. But it is probably harmless to keep it as a "just in case")

ScottDugas · 2025-08-12T20:19:09Z

...ain/java/com/apple/foundationdb/record/provider/foundationdb/OnlineIndexOperationConfig.java

        public Builder setUseSynchronizedSession(boolean useSynchronizedSession) {
-            this.useSynchronizedSession = useSynchronizedSession;
+            // no-op
            return this;
        }

        /**
         * Set the lease length in milliseconds if the synchronized session is used. By default this is {@link #DEFAULT_LEASE_LENGTH_MILLIS}.


This comment is no longer clear

ScottDugas · 2025-08-12T21:12:35Z

...ore/src/main/java/com/apple/foundationdb/record/provider/foundationdb/IndexingHeartbeat.java

+                                if (!otherIndexerId.equals(this.indexerId)) {
+                                    final IndexBuildProto.IndexBuildHeartbeat otherHeartbeat = IndexBuildProto.IndexBuildHeartbeat.parseFrom(kv.getValue());
+                                    final long age = now - otherHeartbeat.getHeartbeatTimeMilliseconds();
+                                    if (age > 0 && age < leaseLength) {


Should this clear the heartbeat if it is expired?
Are you avoiding that to avoid conflicts by a bunch of workers discovering that one has expired?
Does the indexing process guarantee that they are cleared when it fails?

If the instance crashes does the heartbeat stick around until you complete an index build (successfully, or with an exception)?

At this point I believe that every process clears its own heartbeat. Instance crashes should be too rare to cause significant DB space leak (or so I hope). If needed, a cleanup can be triggered by the user with the Indexer's new clearIndexingHeartbeats API.

Saying that, we can clear all heartbeats in FDBRecordStore.clearReadableIndexBuildData, which is triggered when the index becomes readable. What do you think?

✅

Clearing all heartbeats with an index becomes readable.

ScottDugas · 2025-08-12T21:14:46Z

...st/java/com/apple/foundationdb/record/provider/foundationdb/OnlineIndexingHeartbeatTest.java

+        }
+
+        // Verify that the previous clear does not affect other index
+        try (OnlineIndexer indexer = newIndexerBuilder(indexes.get(1)).build()) {


There should be low-level tests of updating leases, and ensuring that the heartbeat stays non-expired.
Also, that it can steal an expired lease. And that the original heartbeat will fail it lost its heartbeat. I think it would only fail if the stealing process is still running so perhaps covering it trying to continue while the stealer is still active, and if that has completed.

ScottDugas · 2025-08-12T21:16:43Z

...yer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/IndexingBase.java

+                        .thenCompose(store -> IndexingHeartbeat.getIndexingHeartbeats(store, common.getPrimaryIndex(), maxCount)));
+    }
+
+    public CompletableFuture<Integer> clearIndexingHeartbeats(long minAgenMilliseconds, int maxIteration) {


Should this use ThrottledRetryingIterator, removing the need for maxIteration?

IDK. The maxIteration is not expected to be used unless something went wrong. Very wrong. Using the ThrottledRetryingIterator might be an overkill.

ScottDugas · 2025-08-12T21:19:15Z

...st/java/com/apple/foundationdb/record/provider/foundationdb/OnlineIndexingHeartbeatTest.java

+class OnlineIndexingHeartbeatTest extends OnlineIndexerTest {
+
+    @Test
+    void testHeartbeatLowLevel() {


It would be good to look at SynchronizedSessionTest and make sure you are covering the same features, probably at a low-level and at a high level.
At first glance, beyond what I've already suggested, I see that it could benefit from testing that two different indexes can be built simultaneously regardless of indexer type.

ScottDugas · 2025-08-12T21:19:37Z

...er-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/OnlineIndexer.java

@@ -1237,7 +1267,7 @@ public static class Builder {
            private DesiredAction ifReadable = DesiredAction.CONTINUE;
            private boolean doAllowUniquePendingState = false;
            private Set<TakeoverTypes> allowedTakeoverSet = null;
-            private long checkIndexingStampFrequency = 60_000;
+            private long checkIndexingStampFrequency = 10_000;


Why check the type stamp more often?

To align the default stamp & heartbeat checking with the default lease time.

jjezra added the breaking change Changes that are not backwards compatible label Aug 4, 2025

jjezra force-pushed the indexing_heartbeat branch from 89d0263 to f318f65 Compare August 7, 2025 15:55

jjezra requested a review from ScottDugas August 8, 2025 17:18

jjezra added 11 commits August 11, 2025 08:01

Set/clear heartbeats at start/end

5462d42

Optimize heartbeat clear. Deprecate use-sync-indexing API

335a03e

false AvoidUsingHardCodedIP warning

6ee3f01

Clear heartbeats after rebuild (whting the same transaction)

ea84f0d

Itarate targets explicitly after rebuild

895defd

thenApply -> whenComplete

7afdb31

Add API get/clear heartbeats

9c3360b

Add 1st test - low level

ab38bae

Add two more tests

5b684e3

Add a comment..

83830c6

jjezra force-pushed the indexing_heartbeat branch from a680857 to 83830c6 Compare August 11, 2025 12:01

jjezra added 2 commits August 11, 2025 08:40

Deprecated sync lock API functions

48a9c29

Some cleanup

521c836

jjezra marked this pull request as ready for review August 11, 2025 14:13

jjezra marked this pull request as draft August 11, 2025 15:06

jjezra added 2 commits August 11, 2025 11:50

At genesis, first check indexing type, then heartbeat

625b66c

Use subspace(...).unpack(..)

a4b9b48

jjezra marked this pull request as ready for review August 11, 2025 20:23

ScottDugas requested changes Aug 12, 2025

View reviewed changes

jjezra added 4 commits August 13, 2025 09:19

Verify no leftover heartbeats after any singleRebuild

9308dfb

wip

4b76a5e

wip

3885faa

wip

4cf86f4

		@@ -180,7 +178,6 @@ <M extends Message> void singleRebuild(
		if (!safeBuild) {

Online Indexer: replace the synchronized runner with a heartbeat #3530

Are you sure you want to change the base?

Online Indexer: replace the synchronized runner with a heartbeat #3530

Conversation

jjezra commented Aug 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ScottDugas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jjezra Aug 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jjezra commented Aug 4, 2025 •

edited

Loading

jjezra Aug 13, 2025 •

edited

Loading