IndexingMerger: lessen work and retry by default #3732

jjezra · 2025-11-06T22:53:23Z

Catching an exception, the IndexingMerger attempts to detect if it should lessen work and retry. Currently it is an include list of known "retyable" errors. If the error is not detected correctly, or not predicted, it will not retry.
Being a background process, however, it is better and safer to retry after timeouts and after any FDB exception.

Resolves #3731

Catching an exception, the IndexingMerger attempts to detect if it should lessen work and retry. Currently it is an include list of known "retyable" errors. If the error is not detected correctly, or not predicted, it will not retry. Being a background process, however, it is better and safer to retry by default. Resolves FoundationDB#3731

ohadzeliger · 2025-11-07T20:27:31Z

...core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/IndexingThrottle.java

+            if (ex == null) {
+                return false;
+            }
+            final Set<Integer> lessenWorkCodes = new HashSet<>(Arrays.asList(


This set can be declared as a static instance - no need to instantiate every time

Just wonder - wouldn't the JVM do the right thing here? It seems like an obvious optimization and I know that other languages are performing this optimization (and much more).

From what I saw, the Java compiler and JIT compiler will do little in terms of optimization of final and it is more intended to convey a design intent and to protect from unintentional change. Setting this to static final would probably be a good way to indicate that this is not changing and prevent extra work that is unneeded.

...r-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/IndexingMerger.java

ScottDugas · 2025-12-01T21:49:55Z

...r-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/IndexingMerger.java

+        // Expecting AsyncToSyncTimeoutException or an instance of TimeoutException. However, cannot
+        // refer to AsyncToSyncTimeoutException without creating a lucene dependency
+        // TODO: remove this kludge
+        if (e.getClass().getCanonicalName().contains("Timeout") ) {


From the PR description:

Being a background process, however, it is better and safer to retry by default.

Why not just have this return false if there is any exception, except InterruptedException.
Here you abort for any exception that doesn't have Timeout in its name, or an FDBException as its cause.

Either way, it seems like having a test with a mock IndexMaintainer that has various failure scenarios would be valuable.

...yer-lucene/src/test/java/com/apple/foundationdb/record/lucene/FDBLuceneIndexFailureTest.java

jjezra · 2025-12-04T21:55:50Z

...er-lucene/src/test/java/com/apple/foundationdb/record/lucene/LuceneIndexMaintenanceTest.java

                }
            };
-            for (int i = 0; i < 100; i++) {
+            for (int i = 0; i < 20; i++) {


@ScottDugas , please verify -

Using now a lower level index maintainer's merge to avoid retries on timeout.

With "100" repetitions the flakyMergeQuick test was reaching the 5 minutes test limit. Had to take it down to 20. Is that acceptable?

I think it would be better to disable all retries in the indexer used for merging.
The point of this test is to validate that if Lucene merge fails randomly in the middle it will still be usable, and not corrupted. Having retries in the IndexingMerger means that it could heal, but we want to make sure that requests coming in while the merge is ongoing don't get a corrupted view.

I'd also recommend on any comments necessary to make that clear.
The javadoc on the test is, apparently, too brief.

...rc/test/java/com/apple/foundationdb/record/provider/foundationdb/OnlineIndexerMergeTest.java

ScottDugas · 2025-12-10T15:51:08Z

...r-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/IndexingMerger.java

+        return e == null ||
+                (IndexingBase.findException(e, RecordCoreTimeoutException.class) == null &&
+                         IndexingBase.findException(e, TimeoutException.class) == null &&
+                         IndexingBase.findException(e, FDBException.class) == null);


Looking at the error codes
It seems like probably the following should not be retried:

batch_transaction_throttled -- this indicates the cluster is overloaded and trying not to do batch-level work. Retrying increases the load

tag_throttled -- Similar, but for a tag (I'm not sure if record layer supports tags, but if we don't know, we may in the future)

There's definitely a long list of errors in there that would be pointless to retry, which is why the original code had a list of error codes where it should lessen work, rather than a list where it shouldn't.

ScottDugas · 2025-12-10T15:53:13Z

...rc/test/java/com/apple/foundationdb/record/provider/foundationdb/OnlineIndexerMergeTest.java

+            future.completeExceptionally(
+                    customTimeout ?
+                    new CustomOperationTimeoutException(exceptionMessage) :
+                    new java.util.concurrent.TimeoutException(exceptionMessage));


This should not be fully qualified, it should use the import

Suggested change

new java.util.concurrent.TimeoutException(exceptionMessage));

new TimeoutException(exceptionMessage));

ScottDugas · 2025-12-10T15:55:34Z

...rc/test/java/com/apple/foundationdb/record/provider/foundationdb/OnlineIndexerMergeTest.java

+        try (OnlineIndexer indexer = OnlineIndexer.newBuilder()
+                .setRecordStoreBuilder(storeBuilder)
+                .setTargetIndexesByName(List.of(INDEX_NAME))
+                .setMaxAttempts(5)


Why are you setting maxAttempts to 5 here?

ScottDugas · 2025-12-10T15:59:06Z

...rc/test/java/com/apple/foundationdb/record/provider/foundationdb/OnlineIndexerMergeTest.java

+        assertTrue(1 < attemptCount.get());
+    }
+
+    private static void adjustMergeControl(final IndexMaintainerState state) {


Nit: This findCause and the CustomOperationTimeoutException seem arbitrarily placed as there are methods above and below that reference them. Generally, in the record layer, nested classes go at the end. helper methods are a bit more flexible, either at the end, or at the end of the tests I think would make sense.

ScottDugas · 2025-12-10T16:01:29Z

...rc/test/java/com/apple/foundationdb/record/provider/foundationdb/OnlineIndexerMergeTest.java

+        }
+    }
+
+    private static <T extends Throwable> T findCause(Throwable throwable, Class<T> causeType) {


Perhaps this could be useful in TestHelpers.

ScottDugas · 2025-12-10T16:06:07Z

...rc/test/java/com/apple/foundationdb/record/provider/foundationdb/OnlineIndexerMergeTest.java

+
+            // Wrap FDBException in multiple layers to test deep unwrapping
+            final CompletableFuture<Void> future = new CompletableFuture<>();
+            FDBException fdbEx = new FDBException("Transaction too old", FDBError.TRANSACTION_TOO_OLD.code());


This is an error code that was explicitly marked as lessening work codes before, should this test be parameterized to:

An errorCode that the runner would retry

An ErrorCode that was in shouldLessen

An ErrorCode that was not, and shouldn't (e.g. no_cluster_file_found)

ScottDugas · 2025-12-10T16:07:18Z

...ecord-layer-lucene/src/main/java/com/apple/foundationdb/record/lucene/LuceneConcurrency.java

     * An exception that is thrown when the async to sync operation times out.
     */
-    public static class AsyncToSyncTimeoutException extends RecordCoreException {
+    public static class AsyncToSyncTimeoutException extends RecordCoreTimeoutException {


Other than backwards compatibility, is there any reason to keep this class around? Should we mark it as API.Status.DEPRECATED, and then, in the future have lucene throw RecordCoreTimeoutException instead?

ScottDugas · 2025-12-10T16:09:59Z

...er-lucene/src/test/java/com/apple/foundationdb/record/lucene/LuceneIndexMaintenanceTest.java

                }
            };
-            for (int i = 0; i < 100; i++) {
+            for (int i = 0; i < 20; i++) {


I think it would be better to disable all retries in the indexer used for merging.
The point of this test is to validate that if Lucene merge fails randomly in the middle it will still be usable, and not corrupted. Having retries in the IndexingMerger means that it could heal, but we want to make sure that requests coming in while the merge is ongoing don't get a corrupted view.

I'd also recommend on any comments necessary to make that clear.
The javadoc on the test is, apparently, too brief.

ScottDugas · 2025-12-10T16:10:33Z

...er-lucene/src/test/java/com/apple/foundationdb/record/lucene/LuceneIndexMaintenanceTest.java

                try {
                    LOGGER.info(KeyValueLogMessage.of("Merge started",
                            "iteration", i));
-                    explicitMergeIndex(dataModel.index, contextProps, dataModel.schemaSetup);


Why not use explicitMergeIndex anymore? A comment in the code would probably be valuable indicating why we're not using the helper method.

ScottDugas · 2025-12-10T16:12:28Z

...rc/test/java/com/apple/foundationdb/record/provider/foundationdb/OnlineIndexerMergeTest.java

+        AtomicInteger attemptCount = new AtomicInteger(0);
+
+        TestFactory.register(indexType, state -> {
+            adjustMergeControl(state);


The code does not retry any errors while opening the store. Are you intentionally leaving that as-is?
Are you intending to change that in a followup?

jjezra added the enhancement New feature or request label Nov 6, 2025

jjezra requested review from ScottDugas and ohadzeliger November 6, 2025 23:04

Backward compatible - abort if the exception is not an FDBException

06ff451

ohadzeliger requested changes Nov 7, 2025

View reviewed changes

jjezra requested a review from ohadzeliger November 7, 2025 21:03

Tmp kludge for testing

168a312

ScottDugas reviewed Dec 1, 2025

View reviewed changes

jjezra added 4 commits December 1, 2025 18:55

Add tests

7633735

Fix a failing test

7b25720

Fix test (tmp)

0c27c3f

use RecordCoreTimeoutException

a1afad4

jjezra commented Dec 4, 2025

View reviewed changes

...yer-lucene/src/test/java/com/apple/foundationdb/record/lucene/FDBLuceneIndexFailureTest.java Show resolved Hide resolved

jjezra commented Dec 4, 2025

View reviewed changes

ohadzeliger reviewed Dec 5, 2025

View reviewed changes

...rc/test/java/com/apple/foundationdb/record/provider/foundationdb/OnlineIndexerMergeTest.java Show resolved Hide resolved

Make lessenWorkCodes a private static var

692f23a

jjezra requested review from ScottDugas and ohadzeliger December 8, 2025 21:49

ohadzeliger approved these changes Dec 9, 2025

View reviewed changes

ScottDugas requested changes Dec 10, 2025

View reviewed changes

	new java.util.concurrent.TimeoutException(exceptionMessage));
	new TimeoutException(exceptionMessage));

IndexingMerger: lessen work and retry by default #3732

Are you sure you want to change the base?

IndexingMerger: lessen work and retry by default #3732

Conversation

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

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jjezra commented Nov 6, 2025 •

edited

Loading