Add bounded parallelism to sorted LSM index builds - #5259
Conversation
|
Tick the box to add this pull request to the merge queue (same as
|
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 146 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
There was a problem hiding this comment.
Code Review
This pull request introduces multi-threaded parallelism to the sorted LSM index build process in ArcadeDB, parallelizing independent sorted-build merge groups and bucket writers while dynamically admitting workers based on system resources. It also adds a comprehensive SortedIndexBuildMetrics class to collect and log detailed diagnostics, alongside extensive tests and benchmarks. The review feedback is highly constructive, identifying a potential ExecutorService leak in the BucketWriteDispatcher constructor if initialization fails, and suggesting a cleaner way to unwrap and propagate exceptions from worker threads in complete() instead of letting ExecutionException wrap the root cause.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| private BucketWriteDispatcher(final Map<LSMTreeIndex, BucketWriter> writers, final int workerCount) { | ||
| this.writers = writers; | ||
| this.queues = new ArrayList<>(workerCount); | ||
| this.pendingBatches = new ArrayList<>(workerCount); | ||
|
|
||
| final List<Integer> bucketIds = new ArrayList<>(indexesByBucket.keySet()); | ||
| bucketIds.sort(Integer::compareTo); | ||
| for (int i = 0; i < bucketIds.size(); i++) | ||
| workerByBucket.put(bucketIds.get(i), i % workerCount); | ||
|
|
||
| final AtomicInteger threadNumber = new AtomicInteger(); | ||
| executor = Executors.newFixedThreadPool(workerCount, task -> { | ||
| final Thread thread = new Thread(task, "arcadedb-index-bucket-writer-" + threadNumber.incrementAndGet()); | ||
| thread.setDaemon(true); | ||
| return thread; | ||
| }); | ||
| for (int i = 0; i < workerCount; i++) { | ||
| final ArrayBlockingQueue<WriteBatch> queue = new ArrayBlockingQueue<>(WRITER_QUEUE_CAPACITY); | ||
| queues.add(queue); | ||
| pendingBatches.add(new WriteBatch(false)); | ||
| futures.add(executor.submit(() -> runWorker(queue))); | ||
| } | ||
| } |
There was a problem hiding this comment.
If an exception is thrown during the initialization of the queues, pending batches, or worker threads (for example, a RejectedExecutionException or OutOfMemoryError), the ExecutorService created at line 492 will be leaked because the constructor fails and the BucketWriteDispatcher is never assigned to a resource variable in the try-with-resources block, preventing close() from being called. Wrapping the initialization loop in a try-catch block and shutting down the executor on failure prevents this resource leak.
private BucketWriteDispatcher(final Map<LSMTreeIndex, BucketWriter> writers, final int workerCount) {
this.writers = writers;
this.queues = new ArrayList<>(workerCount);
this.pendingBatches = new ArrayList<>(workerCount);
final List<Integer> bucketIds = new ArrayList<>(indexesByBucket.keySet());
bucketIds.sort(Integer::compareTo);
for (int i = 0; i < bucketIds.size(); i++)
workerByBucket.put(bucketIds.get(i), i % workerCount);
final AtomicInteger threadNumber = new AtomicInteger();
executor = Executors.newFixedThreadPool(workerCount, task -> {
final Thread thread = new Thread(task, "arcadedb-index-bucket-writer-" + threadNumber.incrementAndGet());
thread.setDaemon(true);
return thread;
});
try {
for (int i = 0; i < workerCount; i++) {
final ArrayBlockingQueue<WriteBatch> queue = new ArrayBlockingQueue<>(WRITER_QUEUE_CAPACITY);
queues.add(queue);
pendingBatches.add(new WriteBatch(false));
futures.add(executor.submit(() -> runWorker(queue)));
}
} catch (final Throwable t) {
executor.shutdownNow();
throw t;
}
}There was a problem hiding this comment.
Fixed in bc3030e. Partial worker initialization now shuts down the executor before the constructor propagates the failure. The 19-test concurrency/failure gate and 42-test broad sorted-build gate both pass.
| executor.shutdown(); | ||
| for (final Future<?> future : futures) | ||
| future.get(); | ||
| checkFailure(); |
There was a problem hiding this comment.
In complete(), calling future.get() directly propagates ExecutionException if a worker thread fails. This wraps the actual cause (e.g., IOException or IndexException) in an ExecutionException, which is then wrapped in another IndexException by the caller, leading to a poor developer experience. Catching ExecutionException and calling checkFailure() allows the actual unwrapped cause to be thrown directly.
executor.shutdown();
for (final Future<?> future : futures) {
try {
future.get();
} catch (final ExecutionException error) {
checkFailure();
throw error;
} catch (final InterruptedException error) {
Thread.currentThread().interrupt();
checkFailure();
throw error;
}
}
checkFailure();There was a problem hiding this comment.
Fixed in bc3030e. complete() now handles ExecutionException through the recorded worker cause and restores the interrupt flag before propagating InterruptedException. Both submission gates remain green.
|
@claude please review this |
|
Claude finished @lvca's task in 3m 32s —— View job Review
I diffed Previously flagged Gemini issuesBoth are correctly resolved in
Minor nit (not blocking)
} catch (final ExecutionException error) {
failure.compareAndSet(null, error.getCause());
checkFailure();
throw error;
}Once Merge-group parallelism (
|
|
Addressed the non-blocking exception-path nit in |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #5259 +/- ##
============================================
+ Coverage 65.45% 66.27% +0.81%
============================================
Files 1703 1660 -43
Lines 139116 133277 -5839
Branches 29816 28876 -940
============================================
- Hits 91064 88327 -2737
+ Misses 35663 32851 -2812
+ Partials 12389 12099 -290 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Another great PR, thanks @justinblethrow-cloud ! |
Bumps `swagger.version` from 2.2.52 to 2.2.53. Updates `io.swagger.core.v3:swagger-core` from 2.2.52 to 2.2.53 Release notes *Sourced from [io.swagger.core.v3:swagger-core's releases](https://github.com/swagger-api/swagger-core/releases).* > Swagger-core 2.2.53 released! > ----------------------------- > > * chore: update Jackson to 2.22.1 ([#5258](https://redirect.github.com/swagger-api/swagger-core/issues/5258)) > * refactor: replace `writer(new DefaultPrettyPrinter())` with `writerWithDefaultPrettyPrinter()` ([#5252](https://redirect.github.com/swagger-api/swagger-core/issues/5252)) > * fix: Stabilize CI Maven and Gradle builds ([#5238](https://redirect.github.com/swagger-api/swagger-core/issues/5238)) > * test: remove system.out.println from tests ([#5236](https://redirect.github.com/swagger-api/swagger-core/issues/5236)) > * chore: bump dependencies ([#5229](https://redirect.github.com/swagger-api/swagger-core/issues/5229)) > * refactor: simplify type handling in ModelDeserializer ([#5227](https://redirect.github.com/swagger-api/swagger-core/issues/5227)) > * Revert "hotfix: temporarily allow Release workflow to skip mvn deploy to recover 2.2.52 ([#5220](https://redirect.github.com/swagger-api/swagger-core/issues/5220))" ([#5223](https://redirect.github.com/swagger-api/swagger-core/issues/5223)) > * chore(deps-dev): bump org.codehaus.groovy:groovy from 3.0.23 to 3.0.25 ([#5216](https://redirect.github.com/swagger-api/swagger-core/issues/5216)) > * chore(deps): bump commons-cli:commons-cli from 1.9.0 to 1.11.0 ([#5209](https://redirect.github.com/swagger-api/swagger-core/issues/5209)) > * chore(deps-dev): bump commons-codec:commons-codec from 1.17.2 to 1.22.0 ([#5208](https://redirect.github.com/swagger-api/swagger-core/issues/5208)) > * fix: emit $ref for array items when cycle guard suppresses implementation processing ([#5205](https://redirect.github.com/swagger-api/swagger-core/issues/5205)) > * Restore inner property name from map key in handleUnwrapped ([#5193](https://redirect.github.com/swagger-api/swagger-core/issues/5193)) > * Honor PropertyNamingStrategy for get/is-prefixed property names ([#5192](https://redirect.github.com/swagger-api/swagger-core/issues/5192)) > * fix: let explicit [`@Schema`](https://github.com/Schema)(format) override type-derived format ([#5185](https://redirect.github.com/swagger-api/swagger-core/issues/5185)) ([#5186](https://redirect.github.com/swagger-api/swagger-core/issues/5186)) > * docs: update format of javadoc to produce a functional link ([#5182](https://redirect.github.com/swagger-api/swagger-core/issues/5182)) > * fix: exclude overridable annotation values when parsing composed annotations ([#5179](https://redirect.github.com/swagger-api/swagger-core/issues/5179)) > * fix: negative and positive validation annotations uses relevant OAS 3.1 syntax ( [#5170](https://redirect.github.com/swagger-api/swagger-core/issues/5170)) ([#5171](https://redirect.github.com/swagger-api/swagger-core/issues/5171)) Commits * [`c4bc5f2`](swagger-api/swagger-core@c4bc5f2) prepare release 2.2.53 ([#5259](https://redirect.github.com/swagger-api/swagger-core/issues/5259)) * [`7d14a26`](swagger-api/swagger-core@7d14a26) refactor: replace `writer(new DefaultPrettyPrinter())` with `writerWithDefaul... * [`b9b2d8c`](swagger-api/swagger-core@b9b2d8c) chore: update Jackson to 2.22.1 ([#5258](https://redirect.github.com/swagger-api/swagger-core/issues/5258)) * [`4aeaa51`](swagger-api/swagger-core@4aeaa51) Honor PropertyNamingStrategy for get/is-prefixed property names ([#5192](https://redirect.github.com/swagger-api/swagger-core/issues/5192)) * [`42ef86b`](swagger-api/swagger-core@42ef86b) fix: preserve array items via $ref when cycle guard suppresses implementation... * [`be0548d`](swagger-api/swagger-core@be0548d) Restore inner property name from map key in handleUnwrapped ([#5193](https://redirect.github.com/swagger-api/swagger-core/issues/5193)) * [`f3de17d`](swagger-api/swagger-core@f3de17d) refactor: simplify type handling in ModelDeserializer ([#5227](https://redirect.github.com/swagger-api/swagger-core/issues/5227)) * [`c4363f5`](swagger-api/swagger-core@c4363f5) fix: exclude overridable annotation values when parsing composed annotations ... * [`6f799ee`](swagger-api/swagger-core@6f799ee) fix: let explicit [`@Schema`](https://github.com/Schema)(format) override type-derived format ([#5185](https://redirect.github.com/swagger-api/swagger-core/issues/5185)) ([#5186](https://redirect.github.com/swagger-api/swagger-core/issues/5186)) * [`604c480`](swagger-api/swagger-core@604c480) chore(deps): bump commons-cli:commons-cli from 1.9.0 to 1.11.0 ([#5209](https://redirect.github.com/swagger-api/swagger-core/issues/5209)) * Additional commits viewable in [compare view](swagger-api/swagger-core@v2.2.52...v2.2.53) Updates `io.swagger.core.v3:swagger-annotations` from 2.2.52 to 2.2.53 Updates `io.swagger.core.v3:swagger-models` from 2.2.52 to 2.2.53
* Add sorted build stage metrics baseline * Add sorted parallelism benchmark * Add bounded sorted bucket writers * Add bounded parallel sorted merge groups * Harden parallel writer failure cleanup * Clarify writer execution failure fallback --------- Co-authored-by: justinblethrow-cloud <226385385+justinblethrow-cloud@users.noreply.github.com> (cherry picked from commit 74faeda)
What does this PR do?
Adds opt-in, resource-admitted parallelism to the sorted
LSM_TREEbuild path introduced in #5244 and extended to unique indexes in #5256.The requested parallelism applies only to work that is already independent:
The default remains one worker. The final global ordered stream remains serial and continues to own key ordering, RID grouping, unique-key enforcement, and publication semantics.
Resource admission and diagnostics
Each stage independently bounds its admitted workers by:
The existing sorted-build metrics now report requested, admitted, and observed merge/writer concurrency alongside the existing run, spill, and stage timing data. A request can therefore fall back to one worker without changing correctness or publication behavior.
Failure and publication behavior
Parallel merge workers own disjoint input cursors, output streams, and serialization buffers. Source runs remain intact until every group in a generation succeeds. On the first failure, the dispatcher cancels peers, waits for worker termination, preserves the first cause, and removes incomplete generation output.
Parallel bucket writers own independent staged bucket-index files. A writer failure cancels and joins its peers before the existing sorted-build cleanup removes staged components and spill artifacts. Schema publication still occurs only after all bucket output succeeds.
No new writer, on-disk layout, recovery mechanism, HA behavior, or online-build path is introduced.
Correctness and validation
Coverage includes:
A full engine run completed 9,347 tests. Every PR3 and adjacent index test passed. It reported three failures outside this patch: one 256-CPU bucket-limit test defect reproduced on untouched
main, plus two shared-suite failures that pass in isolation.Performance evidence
All comparisons use the same binary, fresh fixture copies, a 4 GiB heap, full output digest checks, and reopen validation.
1M, eight source buckets
With a 64 MiB build budget, fan-in 8, and four requested workers:
Both merge and writer stages admitted and observed four concurrent workers.
10M, one source bucket
A serial/parallel/serial bracket isolated materialized merge work because writer admission was one:
Every accepted run produced identical ascending and descending digests. These results describe the tested fixtures and configurations, not a general throughput guarantee.
Scope
This remains an explicit, offline, single-server sorted build. Automatic parallelism, SQL syntax, HA/replicated construction, online snapshot-plus-delta construction, and multi-index scheduling remain out of scope.
Related: #5230.