DO NOT MERGE: #34154: refactor(browser) chunk and parallelise with two gatherers

[fabrizzio-dotCMS]

Why this one

A better proof of concept than the batching case (#37468, now closed): BrowserAPIImpl had both
halves of the gatherer API written by hand, in the same class, twice.

DO NOT MERGE — see the timeouts section, which is the whole finding.

What it replaces

The chunking was a private helper whose entire body was a delegation:

private <T> List<List<T>> createChunks(List<T> list, int chunkSize) {
    return Lists.partition(list, chunkSize);
}

The parallelism was a raw array of futures with a manual index and a counter kept only so the log
line could say which chunk it was:

final CompletableFuture<List<Contentlet>>[] futures = new CompletableFuture[chunkCount];
for (int i = 0; i < chunkCount; i++) {
    final List<String> chunk = chunks.get(i);
    final int chunkIndex = i + 1;
    futures[i] = CompletableFuture.supplyAsync(() -> { ... }, submitter)
        .orTimeout(90, TimeUnit.SECONDS)
        .exceptionally(throwable -> { ... });
}
CompletableFuture.allOf(futures).get(180, TimeUnit.SECONDS);
for (CompletableFuture<List<Contentlet>> future : futures) { allContentlets.addAll(future.get()); }

And hydrateContentletsInParallel did the same again with a future list. Its comment read
"Collect results maintaining order" — ordering it got as a side effect of iterating the futures
in creation order.

What it becomes

static <T, R> List<R> inParallelChunks(final List<T> items, final int chunkSize,
        final int concurrency, final Function<List<T>, List<R>> loader) {

    return items.stream()
            .gather(Gatherers.windowFixed(chunkSize))
            .gather(Gatherers.mapConcurrent(concurrency, loader::apply))
            .flatMap(List::stream)
            .toList();
}

Two gatherers, one concern each: windowFixed cuts and emits the short final chunk itself,
mapConcurrent runs the loaders on virtual threads and promises input order — which
parallelStream() does not. Both call sites now use it.

Net −31 lines (+80/−111).

What it gives up — the finding

mapConcurrent has no timeout of any kind. The shape it replaces carried orTimeout(90s) per
chunk and a 180-second ceiling on the whole set. Neither survives.

Getting them back means either a timeout inside the loader — which puts the ceremony straight back —
or StructuredTaskScope, still preview in Java 25. It is documented in the helper's javadoc, not
quietly dropped, and it is why this stays a draft.

The second thing worth saying out loud: the concurrency bound is the connection pool, not the
CPU. These loaders block on a socket to PostgreSQL, so allowing more chunks in flight than Hikari
has connections just moves the queue from this method into the pool. MAX_CONCURRENT_CHUNKS is
therefore a config property and not availableProcessors().

Tests

Five unit tests, no database:

chunks cover every element, last one short	25 items / size 10 → 10, 10, 5
input order survives any completion order	first chunk sleeps longest; result still 1..20
never exceeds the concurrency bound	limit 3, 30 chunks → peak ≤ 3, and > 1
loaders run on virtual threads	Thread.currentThread().isVirtual()
empty input never calls the loader	0 chunks, 0 invocations

5/5 green; test-compile -pl :dotcms-core --am passes.

Refs #34154

[claude]

Claude finished @fabrizzio-dotCMS's task in 2m 16s —— View job

---

Code Review — #34154 two-gatherer refactor

• Read the diff (BrowserAPIImpl.java, new test)
• Analyze behavior changes (gatherers vs. hand-built futures)
• Post findings

This is a clean, well-argued refactor and the inParallelChunks helper is a genuine improvement over three hand-rolled future loops. The tests are meaningful (order-preservation and the concurrency bound are the right things to pin). Below are the findings beyond the timeout loss you already documented.

New Issues

• 🟠 High: BrowserAPIImpl.java:1042,1096 (blocking, author-acknowledged) — the timeout regression is real and worth stating precisely so it isn't lost. Three timeouts disappear, not two: loadContentletsParallel lost orTimeout(90s) per chunk and the get(180s) global ceiling; hydrateContentletsInParallel also lost its future.get(30, SECONDS) per-chunk timeout (line 1132 area). With mapConcurrent a single hung findContentlets/hydrate now blocks the request until the JDBC socket/statement timeout fires — if none is configured, indefinitely. Agreed this is why it stays a draft; flagging the 30s hydrate timeout because the PR body only mentions the 90s/180s pair.

• 🟡 Medium: BrowserAPIImpl.java:96 — duplicate import java.util.function.Function; (lines 95 and 96). Legal Java (compiles, hence green test-compile) but clearly an accidental leftover. Fix this →

• 🟡 Medium: BrowserAPIImpl.java:63 — import com.google.common.collect.Lists; is now unused (Lists.partition was the only use and it's gone with createChunks). Dead import. Fix this →

• 🟡 Medium: BrowserAPIImpl.java:1038 — MAX_CONCURRENT_CHUNKS is a static final read from Config.getIntProperty(...) once at class-load. Every other Config read in this class (and the dotCMS convention generally) is per-call, so operators can retune without a restart. Freezing it as a constant means browser.contentlet.load.max.concurrent.chunks can never be changed at runtime — which is exactly the pool-sizing knob you argue is the important one. Consider reading it inside loadContentletsParallel/hydrateContentletsInParallel (or passing it in) rather than caching it in a field.

• 🟡 Medium: BrowserAPIImpl.java:1150 — exception-wrapping semantics (verify). The old hydrate path caught ExecutionException from future.get() and re-threw a fresh DotRuntimeException, so callers reliably saw DotRuntimeException. Now the loader throws DotRuntimeException straight through Gatherers.mapConcurrent. Assumption: mapConcurrent re-throws the mapper's exception, but I can't confirm from the diff whether it propagates it unwrapped or wraps it (e.g. in a RuntimeException/CompletionException). What to verify: any caller up the stack that catches DotRuntimeException specifically still works — if mapConcurrent wraps, that catch silently stops matching and error handling changes.

Notes (non-blocking)

• There is a third hand-written future+allOf+timeout block still in this class (~924–969, the inode batch loader) that this refactor doesn't touch — it's the natural next candidate for inParallelChunks if this pattern lands.
• The DB-connection-bound concurrency argument is correct and matches the pre-existing behavior: the old code also ran on DotSubmitter threads, so thread-local propagation isn't a new concern here.

_{· issue-34154-java25-gatherer-mapconcurrent}