pinot: fix cold-cycle data loss, silent OOM partial load, and stop/start race - #2124
Open
KazukiKandaKK wants to merge 2 commits into
Open
KazukiKandaKK wants to merge 2 commits into
KazukiKandaKK wants to merge 2 commits into
Conversation
…d stop/start race pinot/ had three independent bugs, found while running the real ~74GB ClickBench hits.tsv dataset on a c6a.4xlarge (32 GiB RAM) EC2 instance: 1. Cold-cycle data loss (root cause of 0/43 query failures) Pinot's benchmark.sh has effectively run with BENCH_DURABLE=yes since the BENCH_RESTARTABLE -> BENCH_DURABLE rename in commit b282aa4 -- the wrong setting for a system whose loaded state lives only in process memory. Commit b422b2d ("Remove unnecessary BENCH_DURABLE=yes") later deleted the explicit `export BENCH_DURABLE=yes` line, but that's a red herring: BENCH_DURABLE already defaults to `yes` in lib/benchmark-common.sh, so deleting the redundant explicit line changed nothing -- Pinot's effective value was `yes` before that commit and stayed `yes` after it. Pinot's own `-dataDir` persistence flag does not work as documented either (verified locally: throws IllegalStateException on the second QuickStart start, Pinot 1.5.1). Since a full CSV reload takes ~78 minutes and can't run before every one of 43 queries, `load` now sets BENCH_DURABLE=no and re-pushes the existing on-disk segment .tar.gz files via Pinot's official Tar Push API (docs.pinot.apache.org/.../segment-upload) instead of re-parsing hits.tsv from scratch on every cold cycle. 2. Silent partial segment generation under memory pressure pinot-admin.sh defaults to `-Xms4G` with no `-Xmx`, so it falls back to the JVM's default heap ceiling (~25% of physical RAM). On the real 74GB/100-split hits.tsv this OOMed mid-job around segment 60/100 (OutOfMemoryError in SegmentDictionaryCreator) -- but LaunchDataIngestionJob still exited 0 and bench_load's >5GB data-size guard didn't catch it either, since 60 segments already exceeded 5GB. `load` now sets `JAVA_OPTS="-Xms4G -Xmx20G"` and explicitly compares the number of generated segment tars against the number of input splits, failing loudly on any mismatch instead of silently proceeding to the query phase with incomplete data. 3. stop/start race corrupting the next cold cycle `stop` only did `pkill` + a fixed `sleep 2`, but a broker can keep answering for 7-8s after SIGTERM while the JVM shutdown hook is still running, and the embedded ZooKeeper's own port teardown lags even further behind that. The next `start` then either treats the still-alive old broker as "already up" (skipping the real restart) or fails to bind ZooKeeper's port. `stop` now polls until the pinot-admin JVM process itself is fully gone (matched via `pgrep -f 'org.apache.pinot.tools.admin.PinotAdministrator'`) instead of guessing from a single component's HTTP port. `check` similarly no longer treats broker-responsive as "fully started": QuickStart only registers its clean-shutdown hook, and finishes bootstrapping, after printing "Quick start setup complete" to pinot.log; check now greps for that line (scoped to the current invocation via a recorded log offset) in addition to the broker health check, so `stop` is never sent while Pinot is still mid-startup. Verified end-to-end on EC2 (c6a.4xlarge, real 74GB hits.tsv, 100 segments): 37/43 queries now pass across all 3 cold-cycle trials (up from 0/43). The remaining 6 failures are a separate, pre-existing Apache Pinot limitation (standard MIN()/MAX() rejects STRING columns; reported upstream at apache/pinot#19603) unrelated to this benchmark-driver fix.
KazukiKandaKK
had a problem deploying
to
benchmark-approval
September 20, 2026 13:47 — with
GitHub Actions
Error
This was referenced Sep 20, 2026
The fixed -Xms4G -Xmx20G this PR (ClickHouse#2124) added for c6a.4xlarge's OOM broke every smaller machine: ClickHouse/ClickBench's own machine:all CI run on PR ClickHouse#2126 shows QuickStart failing to produce results on c6a.large (4GiB), c6a.xlarge (8GiB), and t3a.small (2GiB) — the JVM can't reserve a 20G/4G heap on hosts with less physical RAM than that. Read /proc/meminfo and size the heap at 64% of RAM, capped at 20G. 64% is anchored so c6a.4xlarge (32GiB, the only machine this fix has been end-to-end verified on: 43/43 queries, 3/3 tries) gets back exactly its already-proven 20G, while smaller machines scale down instead of over-requesting. Not Presto/install's 70% pattern: Pinot keeps segments memory-mapped/off-heap rather than fully in-heap, so a heap that large on machines above 32GiB would fight the OS page cache instead of helping. No 4G floor on -Xms (a 4G floor is exactly what kills a 2-4GiB machine); -Xms only reaches 4G once the machine can spare it. c7a.metal-48xl's failure in the same CI run is NOT addressed here — c8g.metal-48xl (same 384GiB class) succeeded with the old fixed 20G, so a too-small heap can't be the explanation there. That needs its own log-based investigation before any fix is attempted. Reviewed with Grok (grok-4.6) before implementing; verdict was 'appropriate but needs correction' on the original plan (naive Presto-pattern port: 70% ratio + 4G floor on Xms). Both points were folded into this version. Local commit only, not yet pushed to the open PR ClickHouse#2124.
KazukiKandaKK
requested a deployment
to
benchmark-approval
September 21, 2026 01:39 — with
GitHub Actions
Waiting
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
pinot/currently fails all 43 queries (0/43) when run against the real ~74GBhits.tsvon a fresh instance. Found and fixed three separate bugs while tracking this down on a c6a.4xlarge (32 GiB RAM).1. Cold-cycle data loss (root cause of 0/43). Pinot's
benchmark.shhas effectively run withBENCH_DURABLE=yessince theBENCH_RESTARTABLE→BENCH_DURABLErename in commitb282aa49a(2026-05-10) — the wrong setting for a system whose loaded state lives only in process memory. Commitb422b2d4e(2026-06-29) later deleted the explicitexport BENCH_DURABLE=yesline, but that's a red herring:BENCH_DURABLEalready defaults toyesinlib/benchmark-common.sh, so deleting the redundant explicit line changed nothing — Pinot's effective value wasyesbefore that commit and stayedyesafter it.Pinot's own
-dataDirpersistence flag doesn't help either. The Quick Start docs describe it as reloading data across restarts, but I confirmed locally that the second QuickStart start throwsIllegalStateException(Pinot 1.5.1:Preconditions.checkState(quickstartRunnerDir.mkdirs())fails because the directory already exists — reproduced in Docker and confirmed against theQuickstart.javasource).A full CSV reload takes ~78 minutes and can't run before each of the 43 queries, so this PR sets
BENCH_DURABLE=noand hasloadre-push the segment.tar.gzfiles already on disk from the first load, via Pinot's own Tar Push API docs, instead of re-parsinghits.tsvevery cold cycle.2. Silent partial segment generation under memory pressure.
pinot-admin.shdefaults to-Xms4Gwith no-Xmx, so it falls back to the JVM's default heap ceiling (~25% of physical RAM). On the real 74GB/100-splithits.tsvthis OOMed mid-job around segment 60/100, butLaunchDataIngestionJobstill exited 0 and the >5GB data-size guard inbench_loaddidn't catch it either, since 60 segments already cleared 5GB.loadnow sizes the JVM heap dynamically instead of hard-coding one value, and compares the number of generated segment tars against the number of input splits, failing loudly on any mismatch.Update (2026-09-21): this originally set a fixed
JAVA_OPTS="-Xms4G -Xmx20G", sized for the c6a.4xlarge this PR was verified on. ClickBench's ownmachine:allCI run on this PR's follow-up (#2126) showed that fixed value breaking every smaller machine —c6a.large(4GiB),c6a.xlarge(8GiB), andt3a.small(2GiB) all failed to even start the JVM, since it can't reserve a 20G/4G heap on a host with less physical RAM than that.start/loadnow read/proc/meminfoand size the heap at 64% of RAM capped at 20G — 64% is anchored so c6a.4xlarge (32GiB, the only machine this fix has been end-to-end verified on: 37/43 queries, 3/3 cold-cycle trials) gets back exactly the already-proven 20G, while smaller machines scale down instead of over-requesting; larger machines stay capped at 20G rather than growing further, since Pinot keeps segments memory-mapped/off-heap and a larger heap would fight the OS page cache instead of helping. Re-verified end-to-end on a freshc6a.large(4GiB): QuickStart now starts (-Xms2425m -Xmx2425m), completes bootstrap, answers queries, and stops cleanly — all of which failed outright with the old fixed value.Note:
c7a.metal-48xl's failure in the same CI run is a separate, unaddressed issue —c8g.metal-48xl(same 384GiB class) succeeded with the old fixed 20G heap, so an undersized heap isn't a sufficient explanation there. That needs its own investigation.3. stop/start race corrupting the next cold cycle.
stoponly didpkillplus a fixedsleep 2, but the broker can keep answering for 7-8s after SIGTERM while the JVM shutdown hook is still running, and the embedded ZooKeeper's port teardown lags even further behind that. The nextstartthen either treats the still-alive old broker as "already up" (skipping the real restart) or fails to bind ZooKeeper's port.stopnow polls until thepinot-adminJVM process itself is fully gone.checksimilarly no longer treats broker-responsive as "fully started" — QuickStart only finishes bootstrapping (and registers its clean-shutdown hook) after printing "Quick start setup complete" topinot.log, sochecknow greps for that line too, scoped to the current invocation via a recorded log offset.Verified end-to-end on EC2 (c6a.4xlarge, real 74GB
hits.tsv, 100 segments): 37/43 queries pass across all 3 cold-cycle trials, up from 0/43. The remaining 6 failures are a separate, pre-existing Pinot limitation (standardMIN()/MAX()rejects STRING columns) unrelated to this fix, reported upstream at apache/pinot#19603. A follow-up PR here addresses those once the query-side workaround is ready.