From 187013d04d502a1c0322aa74b4a03fd799b06cb4 Mon Sep 17 00:00:00 2001 From: Kazuki Kanda Date: Sun, 20 Sep 2026 10:37:13 +0900 Subject: [PATCH 1/2] Fix Pinot benchmark cold-cycle data loss, OOM silent partial load, and 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 b282aa49a -- the wrong setting for a system whose loaded state lives only in process memory. Commit b422b2d4e ("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. --- pinot/benchmark.sh | 23 ++++++++++++ pinot/check | 26 +++++++++++-- pinot/load | 91 ++++++++++++++++++++++++++++++++++++++++++---- pinot/start | 18 ++++++++- pinot/stop | 23 +++++++++++- 5 files changed, 168 insertions(+), 13 deletions(-) diff --git a/pinot/benchmark.sh b/pinot/benchmark.sh index e5db3195ea..f0515a7577 100755 --- a/pinot/benchmark.sh +++ b/pinot/benchmark.sh @@ -4,6 +4,29 @@ export BENCH_DOWNLOAD_SCRIPT="download-hits-tsv" # inside one JVM and takes longer than the lib's 300 s default to be # query-ready on a cold instance. 900 s clears the observed cold start. export BENCH_CHECK_TIMEOUT=900 +# Pinot QuickStart's -dataDir persistence is broken in 1.5.0/1.5.1: the +# second QuickStart invocation against an existing -dataDir always dies +# with IllegalStateException from Quickstart.java's +# Preconditions.checkState(quickstartRunnerDir.mkdirs()) — mkdirs() returns +# false because the directory from the first run already exists (verified +# by reproducing this locally in Docker and reading the Pinot source). +# A stop/start cycle therefore always wipes the in-memory table/segment +# registration and there is no supported way to bring the daemon back up +# against its previous state directory. +# +# What *does* survive the cycle is the segment tar files QuickStart wrote +# to a plain local directory (batch/hits/segments/, not process memory — +# verified locally: kill -9 the JVM, restart it, the .tar.gz files are +# still on disk). BENCH_DURABLE=no makes the shared driver re-run ./load +# after every restart; our ./load exploits this by re-pushing those +# existing tars straight to the controller's segment-upload API +# (https://docs.pinot.apache.org/build-with-pinot/ingestion/batch-ingestion/segment-upload) +# instead of re-parsing hits.tsv, so the "reload" on cold tries 2-129 +# (43 queries x 3 tries) takes seconds instead of the ~78 min a real +# from-source reload would need. This keeps the real stop/start/ +# drop_caches cycle other systems get, rather than skipping it via +# BENCH_RESTARTABLE=no. +export BENCH_DURABLE=no # Skip the pre-snapshot ./stop+./start cycle: the loaded # state lives only in the daemon's process memory (in-process # DataFrame, JVM heap caches) and stopping wipes it. The diff --git a/pinot/check b/pinot/check index 3bfe104c3f..ac6b50fc09 100755 --- a/pinot/check +++ b/pinot/check @@ -1,10 +1,28 @@ #!/bin/bash set -e -# Pinot is responsive once both the controller and the broker accept queries. -RES=$(curl -sf -o /dev/null -w '%{http_code}' \ +# The QuickStart process only registers its clean-shutdown hook after +# ./start's launched process finishes starting every component AND +# bootstrapping every sample table (including waiting for each table's +# async minion segment-generation-and-push task to finish). If ./stop +# sends SIGTERM any earlier than that, either shutdown-hook registration +# itself throws (component startup still in flight) or an in-flight +# minion task gets killed (bootstrap still in flight) -- either way the +# process dies without a clean stop and corrupts the next ./start. +# +# Broker readiness alone is not a safe proxy for "fully done starting": +# it comes up long before bootstrap finishes. The only reliable signal is +# the "Quick start setup complete" line QuickStart itself prints right +# after runner.bootstrapTable() + waitForBootstrapToComplete() return +# (see pinot-tools Quickstart.java, method execute()). Since pinot.log is +# appended across restarts, only look at what was appended after the +# offset ./start recorded for this invocation. +BROKER_RES=$(curl -sf -o /dev/null -w '%{http_code}' \ -XPOST -H'Content-Type: application/json' \ http://localhost:8000/query/sql/ \ - -d '{"sql":"SELECT 1"}') + -d '{"sql":"SELECT 1"}' 2>/dev/null || echo "000") -[ "$RES" = "200" ] +OFFSET=$(cat pinot.log.offset 2>/dev/null || echo 0) +SETUP_DONE=$(tail -c +"$((OFFSET + 1))" pinot.log 2>/dev/null | grep -c 'Quick start setup complete' || true) + +[ "$BROKER_RES" = "200" ] && [ "${SETUP_DONE:-0}" -ge 1 ] diff --git a/pinot/load b/pinot/load index 7229254546..da2c1b3a22 100755 --- a/pinot/load +++ b/pinot/load @@ -3,6 +3,7 @@ set -e PINOT_VERSION=1.5.1 PINOT_DIR="apache-pinot-$PINOT_VERSION-bin" +SEGMENTS_DIR="batch/hits/segments" # Wait for the controller's REST API to be live. `start` only waits for # the broker; the controller can lag, and AddTable below talks to the @@ -32,15 +33,68 @@ for _ in $(seq 1 60); do sleep 1 done -# Register the schema and table. Previously trailed `|| true`, which -# masked genuine failures (controller not up, bad JSON, etc.) and let -# LaunchDataIngestionJob run against a missing table — the symptom -# was a misleading "Failed to fetch hits/schema" deep in the ingestion -# job. Fail loudly here. -"./${PINOT_DIR}/bin/pinot-admin.sh" AddTable \ +# Register the schema and table. AddTable fails with "already exists" on +# every call after the first (BENCH_DURABLE=no re-runs ./load on every +# cold cycle; QuickStart's own restart wipes the table registration but +# not the segment tar files on disk — see the fast path below). Treat +# that specific failure as success; anything else should still fail loudly +# as before. +add_table_out=$("./${PINOT_DIR}/bin/pinot-admin.sh" AddTable \ -tableConfigFile offline_table.json \ - -schemaFile schema.json -exec + -schemaFile schema.json -exec 2>&1) && add_table_rc=0 || add_table_rc=$? +if [ "$add_table_rc" -ne 0 ]; then + if ! printf '%s\n' "$add_table_out" | grep -q 'already exists'; then + printf '%s\n' "$add_table_out" >&2 + exit "$add_table_rc" + fi +fi + +# Fast path: segment tar files from a previous ./load already exist on +# disk. QuickStart -type batch writes them under $SEGMENTS_DIR (a plain +# local directory, not process memory) and a stop/start cycle wipes only +# the in-memory table/segment registration, not these files (verified by +# reproducing this locally in Docker: kill -9 the QuickStart JVM, restart +# it, curl POST each existing .tar.gz straight to the controller's +# /segments endpoint — official "tar push" upload API, see +# https://docs.pinot.apache.org/build-with-pinot/ingestion/batch-ingestion/segment-upload +# — and the table is fully queryable again, star-tree index included, +# without re-parsing hits.tsv). Re-pushing ~100 existing tars takes +# seconds; re-running LaunchDataIngestionJob from hits.tsv takes ~78 min. +if [ -d "$SEGMENTS_DIR" ] && ls "$SEGMENTS_DIR"/*.tar.gz >/dev/null 2>&1; then + for f in "$SEGMENTS_DIR"/*.tar.gz; do + curl -sS -X POST -H 'Content-Type: multipart/form-data' \ + -F "file=@${f}" \ + 'http://localhost:9000/segments?tableName=hits&tableType=OFFLINE' \ + | grep -q 'Successfully uploaded' || { + echo "bench: failed to re-push ${f}" >&2 + exit 1 + } + done + + # Re-pushed segments take a few seconds to show up in the external + # view (Helix propagation), during which queries return a transient + # "N segments unavailable" error. Poll until every expected segment + # is ONLINE instead of racing the very first query against it + # (observed locally: up to ~20s for 5 segments; sized generously + # for ~100). + expected=$(ls "$SEGMENTS_DIR"/*.tar.gz | wc -l | tr -d ' ') + for _ in $(seq 1 120); do + online=$(curl -sf http://localhost:9000/tables/hits/externalview 2>/dev/null \ + | python3 -c 'import json,sys +d=json.load(sys.stdin) +ov=d.get("OFFLINE") or {} +print(sum(1 for st in ov.values() if list(st.values()) == ["ONLINE"]))' \ + 2>/dev/null || echo 0) + if [ "$online" -ge "$expected" ]; then + break + fi + sleep 1 + done + exit 0 +fi + +# Slow path (first ./load only): generate segments from hits.tsv. # Pinot was unable to load data as a single file without errors. Split. split -d --additional-suffix .tsv -n l/100 hits.tsv parts @@ -51,7 +105,30 @@ sed parts93.tsv -e 's/"tatuirovarki_redmond/tatuirovarki_redmond/g' -i sed splitted.yaml -e "s|PWD_DIR_PLACEHOLDER|$PWD|g" -i sed local.yaml -e "s|PWD_DIR_PLACEHOLDER|$PWD|g" -i +# pinot-admin.sh's own default is -Xms4G with NO -Xmx (falls back to the +# JVM's default of ~25% of physical RAM). On a real ~74GB/100-segment +# ClickBench hits.tsv this OOMed mid-job around segment 60/100 on a +# 30GB-RAM c6a.4xlarge (OutOfMemoryError in SegmentDictionaryCreator) — +# and LaunchDataIngestionJob still exited 0, silently leaving only 60/100 +# segments on disk. Force a real heap ceiling sized for this instance. +export JAVA_OPTS="-Xms4G -Xmx20G" + "./${PINOT_DIR}/bin/pinot-admin.sh" LaunchDataIngestionJob -jobSpecFile splitted.yaml +# Defense against the exact failure this instance just hit: Pinot's own +# ingestion job runner can swallow a per-file OutOfMemoryError (or other +# per-task exception) and still exit 0, leaving fewer segment tars on +# disk than input splits. Fail loudly instead of letting a partial load +# silently pass bench_load's >5GB data-size check downstream. +actual_segments=$(ls "$SEGMENTS_DIR"/*.tar.gz 2>/dev/null | wc -l | tr -d ' ') +expected_segments=$(ls parts*.tsv 2>/dev/null | wc -l | tr -d ' ') +if [ "$expected_segments" -gt 0 ] && [ "$actual_segments" -ne "$expected_segments" ]; then + echo "bench: LaunchDataIngestionJob produced ${actual_segments}/${expected_segments} segments — partial load (likely OOM swallowed per-task)" >&2 + exit 1 +fi + +# Keep hits.tsv's derived parts only as long as needed; the source file +# itself and the split parts are no longer needed once segments exist in +# $SEGMENTS_DIR (used by the fast path above on every later cold cycle). rm -f hits.tsv parts*.tsv sync diff --git a/pinot/start b/pinot/start index bd189e8077..b18733d796 100755 --- a/pinot/start +++ b/pinot/start @@ -4,7 +4,9 @@ set -e PINOT_VERSION=1.5.1 PINOT_DIR="apache-pinot-$PINOT_VERSION-bin" -# Idempotent: if broker query endpoint is up, do nothing. +# Idempotent: if broker query endpoint is up, do nothing (leave the +# existing pinot.log offset marker as-is; ./check will find the earlier +# "setup complete" marker in the untouched log). if curl -sf -o /dev/null -w '%{http_code}' \ -XPOST -H'Content-Type: application/json' \ http://localhost:8000/query/sql/ \ @@ -12,6 +14,20 @@ if curl -sf -o /dev/null -w '%{http_code}' \ exit 0 fi +# Record the current end of pinot.log so ./check can look only at what +# this invocation appends, not a "setup complete" line left over from an +# earlier run of the log-appending (>>) QuickStart process. +touch pinot.log +wc -c < pinot.log | tr -d ' ' > pinot.log.offset + +# pinot-admin.sh's own default is -Xms4G with NO -Xmx (falls back to the +# JVM's default of ~25% of physical RAM). On a 100-segment/~74GB ClickBench +# run this OOMed mid-LaunchDataIngestionJob around segment 60/100 on a +# 30GB-RAM c6a.4xlarge (observed: OutOfMemoryError in +# SegmentDictionaryCreator, job silently reported success anyway). +# Force a real heap ceiling sized for this instance. +export JAVA_OPTS="-Xms4G -Xmx20G" + nohup "./${PINOT_DIR}/bin/pinot-admin.sh" QuickStart -type batch \ >> pinot.log 2>&1 < /dev/null & disown diff --git a/pinot/stop b/pinot/stop index 140a589693..cbfab09ec6 100755 --- a/pinot/stop +++ b/pinot/stop @@ -3,5 +3,26 @@ pkill -f 'pinot-admin' 2>/dev/null || true pkill -f 'pinot.tools.admin' 2>/dev/null || true pkill -f 'org.apache.pinot' 2>/dev/null || true -sleep 2 + +# pkill only sends SIGTERM; the actual teardown (JVM shutdown hook +# stopping controller/broker/server, then the embedded Zookeeper +# releasing its port) is asynchronous and takes several seconds. Two +# things were previously observed to still be alive well after a fixed +# short sleep or even after the broker itself stopped responding: +# - the broker itself (was seen answering 200 for ~7-8s after SIGTERM) +# - the embedded Zookeeper's port 2123, which a fixed sleep or a +# broker-only check does not account for; the next ./start's fresh +# Zookeeper can then fail to bind/connect on the still-held port, +# surfacing as "ZkTimeoutException: Unable to connect to zookeeper +# server within timeout" in the next start's controller. +# The only reliable signal that teardown is actually finished is that +# the pinot-admin JVM process itself is gone. Poll for that (up to 30s) +# instead of guessing a delay or checking a single port. +for _ in $(seq 1 30); do + if ! pgrep -f 'org.apache.pinot.tools.admin.PinotAdministrator' > /dev/null 2>&1; then + break + fi + sleep 1 +done + exit 0 From ecd0e2dea420e192da0be14d6738639e0f8bdbe2 Mon Sep 17 00:00:00 2001 From: Kazuki Kanda Date: Sun, 20 Sep 2026 22:48:06 +0900 Subject: [PATCH 2/2] pinot: work around STRING MIN/MAX limitation in queries.sql (apache/pinot#19603) Depends on #2124 (cold-cycle fix) -- without it, pinot/ fails all 43 queries before this even matters. With that fix applied, 6 of the 43 queries still fail. All 6 use MIN/MAX/extract/DATE_TRUNC on EventTime, a STRING column with a SIMPLE_DATE_FORMAT dateTimeFieldSpec. Standard SQL MIN/MAX in Pinot always compiles to a numeric-only aggregation function regardless of the column's declared type, confirmed in AggregationFunctionType (pinot-segment-spi): // TODO: min/max only supports NUMERIC in Pinot, where Calcite // supports COMPARABLE_ORDERED MIN("min", SqlTypeName.DOUBLE, SqlTypeName.DOUBLE), MAX("max", SqlTypeName.DOUBLE, SqlTypeName.DOUBLE), NonScanBasedAggregationOperator's fast aggregation path then calls toDouble() unconditionally on the dictionary value, which throws NumberFormatException: For input string: "2013-07-01". Pinot already ships a fix for this (apache/pinot#16980, merged 2025-10-10): an AggregateFunctionRewriteOptimizer that rewrites MIN/MAX on a STRING column to MINSTRING/MAXSTRING. A follow-up (apache/pinot#17058, merged 2025-10-30) gated the rewrite behind a query option: its diff adds `if (Boolean.parseBoolean(options.get(QueryOptionKey.AUTO_REWRITE_AGGREGATION_TYPE))) { useRuleSet.add(...AGGREGATE_FUNCTION_REWRITE); }`, so autoRewriteAggregationType defaults to off when unset. Confirmed Pinot 1.5.1 (used here) postdates both merges. Filed apache/pinot#19603 upstream, since neither the option nor MINSTRING/MAXSTRING is mentioned on the Query Options doc page and the error message gives no hint they exist. This PR turns the option on for every ClickBench query: - pinot/query: adds autoRewriteAggregationType=true - pinot/queries.sql: EventTime is used directly in extract(minute FROM EventTime) and DATE_TRUNC('minute', EventTime) in queries 19 and 43. Those aren't MIN/MAX, so the option doesn't touch them; they fail for the same underlying reason (Calcite treats the STRING column as non-temporal). Wrapped both in CAST(EventTime AS TIMESTAMP), matching the existing pattern in this benchmark's sail/sail-partitioned queries.sql, which already do the exact same cast for these same two queries. Verified locally (Docker, Pinot 1.5.1, 3-row STRING table) that OPTION(autoRewriteAggregationType=true) alone fixes the MIN/MAX case, and on EC2 that all 6 previously-failing queries now return non-null results with this change plus the cold-cycle fix from #2124. --- pinot/queries.sql | 4 ++-- pinot/query | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/pinot/queries.sql b/pinot/queries.sql index 31f65fc898..47114dee7f 100644 --- a/pinot/queries.sql +++ b/pinot/queries.sql @@ -16,7 +16,7 @@ SELECT SearchEngineID, SearchPhrase, COUNT(*) AS c FROM hits WHERE SearchPhrase SELECT UserID, COUNT(*) FROM hits GROUP BY UserID ORDER BY COUNT(*) DESC LIMIT 10; SELECT UserID, SearchPhrase, COUNT(*) FROM hits GROUP BY UserID, SearchPhrase ORDER BY COUNT(*) DESC LIMIT 10; SELECT UserID, SearchPhrase, COUNT(*) FROM hits GROUP BY UserID, SearchPhrase LIMIT 10; -SELECT UserID, extract(minute FROM EventTime) AS m, SearchPhrase, COUNT(*) FROM hits GROUP BY UserID, m, SearchPhrase ORDER BY COUNT(*) DESC LIMIT 10; +SELECT UserID, extract(minute FROM CAST(EventTime AS TIMESTAMP)) AS m, SearchPhrase, COUNT(*) FROM hits GROUP BY UserID, m, SearchPhrase ORDER BY COUNT(*) DESC LIMIT 10; SELECT UserID FROM hits WHERE UserID = 435090932899640449; SELECT COUNT(*) FROM hits WHERE URL LIKE '%google%'; SELECT SearchPhrase, MIN(URL), COUNT(*) AS c FROM hits WHERE URL LIKE '%google%' AND SearchPhrase <> '' GROUP BY SearchPhrase ORDER BY c DESC LIMIT 10; @@ -40,4 +40,4 @@ SELECT URL, COUNT(*) AS PageViews FROM hits WHERE CounterID = 62 AND EventDate > SELECT TraficSourceID, SearchEngineID, AdvEngineID, CASE WHEN (SearchEngineID = 0 AND AdvEngineID = 0) THEN Referer ELSE '' END AS Src, URL AS Dst, COUNT(*) AS PageViews FROM hits WHERE CounterID = 62 AND EventDate >= '2013-07-01' AND EventDate <= '2013-07-31' AND IsRefresh = 0 GROUP BY TraficSourceID, SearchEngineID, AdvEngineID, Src, Dst ORDER BY PageViews DESC LIMIT 10 OFFSET 1000; SELECT URLHash, EventDate, COUNT(*) AS PageViews FROM hits WHERE CounterID = 62 AND EventDate >= '2013-07-01' AND EventDate <= '2013-07-31' AND IsRefresh = 0 AND TraficSourceID IN (-1, 6) AND RefererHash = 3594120000172545465 GROUP BY URLHash, EventDate ORDER BY PageViews DESC LIMIT 10 OFFSET 100; SELECT WindowClientWidth, WindowClientHeight, COUNT(*) AS PageViews FROM hits WHERE CounterID = 62 AND EventDate >= '2013-07-01' AND EventDate <= '2013-07-31' AND IsRefresh = 0 AND DontCountHits = 0 AND URLHash = 2868770270353813622 GROUP BY WindowClientWidth, WindowClientHeight ORDER BY PageViews DESC LIMIT 10 OFFSET 10000; -SELECT DATE_TRUNC('minute', EventTime) AS M, COUNT(*) AS PageViews FROM hits WHERE CounterID = 62 AND EventDate >= '2013-07-14' AND EventDate <= '2013-07-15' AND IsRefresh = 0 AND DontCountHits = 0 GROUP BY DATE_TRUNC('minute', EventTime) ORDER BY DATE_TRUNC('minute', EventTime) LIMIT 10 OFFSET 1000; +SELECT DATE_TRUNC('minute', CAST(EventTime AS TIMESTAMP)) AS M, COUNT(*) AS PageViews FROM hits WHERE CounterID = 62 AND EventDate >= '2013-07-14' AND EventDate <= '2013-07-15' AND IsRefresh = 0 AND DontCountHits = 0 GROUP BY DATE_TRUNC('minute', CAST(EventTime AS TIMESTAMP)) ORDER BY DATE_TRUNC('minute', CAST(EventTime AS TIMESTAMP)) LIMIT 10 OFFSET 1000; diff --git a/pinot/query b/pinot/query index 337792437a..d8b068644a 100755 --- a/pinot/query +++ b/pinot/query @@ -12,7 +12,7 @@ query=$(printf '%s' "$query" | tr -d ';') req=$(printf '%s' "$query" | python3 -c ' import json, sys q = sys.stdin.read() -print(json.dumps({"sql": q + " option(timeoutMs=300000)"})) +print(json.dumps({"sql": q + " option(timeoutMs=300000,autoRewriteAggregationType=true)"})) ') resp=$(curl -sS -XPOST -H'Content-Type: application/json' \