Make Lucene partitioner thread-safe #3699

ohadzeliger · 2025-10-28T17:22:42Z

Lucene partitioner is not thread safe when records are updated concurrently.
The partition count and boundaries are not thread safe and can skew in multi threaded updates.
The changes include protecting the partition metadata changes with a keyspace lock.
The LuceneIndexMaintenanceTest.concurrent* tests have been extended to run in partitioned setup and are now passing.
In addition, a bug where empty partitions were not removed was uncovered, and a fix was introduced, with accompanying tests. Empty partitions (first, middle and last) are removed with their index data, ensuring at least one partition is always left behind.

Resolves #2990

This changes the concurrentUpdate to run with a single partition, and adds usages of AsyncLock to ensure that updates to the partition metadata work correctly. Before calling the issue complete an additional test should be added that does concurrent inserts, and one for concurrent deletes.

...cord-layer-core/src/main/java/com/apple/foundationdb/record/RecordCoreInternalException.java

...d-layer-lucene/src/main/java/com/apple/foundationdb/record/lucene/LuceneIndexMaintainer.java

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

...ayer-lucene/src/test/java/com/apple/foundationdb/record/lucene/LuceneIndexTestValidator.java

…ord/RecordCoreInternalException.java Co-authored-by: Scott Dugas <scott.dugas@gmail.com>

Code restructuring to makemore readable Verify that partition is empty before removal Added tests

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

fdb-record-layer-lucene/src/test/java/com/apple/foundationdb/record/lucene/LuceneIndexTest.java

...d-layer-lucene/src/test/java/com/apple/foundationdb/record/lucene/LucenePartitionerTest.java

ScottDugas · 2025-11-06T17:37:05Z

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

                assertThat(partitionCounts, Matchers.contains(5, 3, 4)));
    }

+    static Stream<Arguments> removeEmptyPartitions() {


I think the new assertions there are good, but that is different from the test I was describing.
concurrentDelete only does one merge, after deleting all the records, and I was suggesting a loop:

while (records.size() > 0) { delete(random.nextInt(records.size())); commit merge validate }

This will give better coverage of the relationship between merge and various partial states, as opposed to all partitions being gone.

ScottDugas · 2025-11-06T17:39:11Z

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

-        } else {
-            timerSnapshot = null;
-        }
+        final StoreTimerSnapshot timerSnapshot = getStoreTimerSnapshot();


I think we should mark this teamscale warning as tolerated.
The line at which we call getStoreTimerSnapshot is important, and calling it later would not give us timing information.

added test to randomly remove records and assert empty partitions.

This changes the concurrentUpdate to run with a single partition, and adds usages of AsyncLock to ensure that updates to the partition metadata work correctly. Before calling the issue complete an additional test should be added that does concurrent inserts, and one for concurrent deletes.

…ord/RecordCoreInternalException.java Co-authored-by: Scott Dugas <scott.dugas@gmail.com>

Code restructuring to makemore readable Verify that partition is empty before removal Added tests

added test to randomly remove records and assert empty partitions.

…read-safe-partitioner # Conflicts: # fdb-record-layer-lucene/src/test/java/com/apple/foundationdb/record/lucene/LuceneIndexMaintenanceTest.java

github-actions · 2025-11-07T15:00:14Z

📊 Metrics Diff Analysis Report

Summary

New queries: 0
Dropped queries: 0
Plan changed + metrics changed: 0
Plan unchanged + metrics changed: 0

ℹ️ About this analysis

This automated analysis compares query planner metrics between the base branch and this PR. It categorizes changes into:

New queries: Queries added in this PR
Dropped queries: Queries removed in this PR. These should be reviewed to ensure we are not losing coverage.
Plan changed + metrics changed: The query plan has changed along with planner metrics.
Metrics only changed: Same plan but different metrics

The last category in particular may indicate planner regressions that should be investigated.

…read-safe-partitioner

ScottDugas · 2025-11-07T20:10:19Z

...-lucene/src/test/java/com/apple/foundationdb/record/lucene/LuceneRepartitionPlannerTest.java

 @Tag(Tags.RequiresFDB)
 public class LuceneRepartitionPlannerTest {
+
+    private static Stream<Arguments> luceneRepartitionPlannerTest() {


I'm surprised this can be private and junit can handle it, but it seems like it can.

ScottDugas and others added 4 commits October 23, 2025 15:28

Parameterize tests, add handling of removing empty partitions

43b157c

Fix issue with empty partitions not being removed, add tests to verify

2e36a01

cleanup

6046898

ohadzeliger self-assigned this Oct 28, 2025

ohadzeliger added the bug fix Change that fixes a bug label Oct 28, 2025

ohadzeliger requested a review from ScottDugas October 28, 2025 17:59

ScottDugas requested changes Nov 3, 2025

View reviewed changes

ohadzeliger and others added 2 commits November 4, 2025 13:28

Update fdb-record-layer-core/src/main/java/com/apple/foundationdb/rec…

83350d2

…ord/RecordCoreInternalException.java Co-authored-by: Scott Dugas <scott.dugas@gmail.com>

PR comments.

2b2bf00

Code restructuring to makemore readable Verify that partition is empty before removal Added tests

ScottDugas requested changes Nov 6, 2025

View reviewed changes

ohadzeliger and others added 10 commits November 6, 2025 16:51

PR comments.

3e9ba27

added test to randomly remove records and assert empty partitions.

Parameterize tests, add handling of removing empty partitions

d210a6c

Fix issue with empty partitions not being removed, add tests to verify

b3fd29c

cleanup

dae27d7

Update fdb-record-layer-core/src/main/java/com/apple/foundationdb/rec…

88e3e9b

…ord/RecordCoreInternalException.java Co-authored-by: Scott Dugas <scott.dugas@gmail.com>

PR comments.

dc1cbc8

Code restructuring to makemore readable Verify that partition is empty before removal Added tests

PR comments.

97de052

added test to randomly remove records and assert empty partitions.

Rebase from main

e9cb266

Merge remote-tracking branch 'origin/thread-safe-partitioner' into th…

1e2a0b1

…read-safe-partitioner # Conflicts: # fdb-record-layer-lucene/src/test/java/com/apple/foundationdb/record/lucene/LuceneIndexMaintenanceTest.java

ohadzeliger and others added 4 commits November 7, 2025 10:00

Merge branch 'main' into thread-safe-partitioner

e06d4f8

Fix broken test

d9de92a

Merge remote-tracking branch 'origin/thread-safe-partitioner' into th…

3698994

…read-safe-partitioner

Indentation

d47643f

ScottDugas approved these changes Nov 7, 2025

View reviewed changes

ohadzeliger merged commit f032a91 into FoundationDB:main Nov 7, 2025
8 checks passed

ohadzeliger deleted the thread-safe-partitioner branch November 7, 2025 21:34

Make Lucene partitioner thread-safe #3699

Make Lucene partitioner thread-safe #3699

Uh oh!

Conversation

ohadzeliger commented Oct 28, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ScottDugas Nov 6, 2025

Choose a reason for hiding this comment

Uh oh!

ScottDugas Nov 6, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Nov 7, 2025

📊 Metrics Diff Analysis Report

Summary

Uh oh!

ScottDugas Nov 7, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants