Visitar URL original
Fix Pinot benchmark cold-cycle data loss, OOM silent partial load, an… · ClickHouse/ClickBench@187013d · GitHub
Skip to content

Commit 187013d

Browse files
KazukiKandaKKKazukiKandaKK
committed
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 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.
1 parent b1134e0 commit 187013d

5 files changed

Lines changed: 168 additions & 13 deletions

File tree

‎pinot/benchmark.sh‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,29 @@ export BENCH_DOWNLOAD_SCRIPT="download-hits-tsv"
44
# inside one JVM and takes longer than the lib's 300 s default to be
55
# query-ready on a cold instance. 900 s clears the observed cold start.
66
export BENCH_CHECK_TIMEOUT=900
7+
# Pinot QuickStart's -dataDir persistence is broken in 1.5.0/1.5.1: the
8+
# second QuickStart invocation against an existing -dataDir always dies
9+
# with IllegalStateException from Quickstart.java's
10+
# Preconditions.checkState(quickstartRunnerDir.mkdirs()) — mkdirs() returns
11+
# false because the directory from the first run already exists (verified
12+
# by reproducing this locally in Docker and reading the Pinot source).
13+
# A stop/start cycle therefore always wipes the in-memory table/segment
14+
# registration and there is no supported way to bring the daemon back up
15+
# against its previous state directory.
16+
#
17+
# What *does* survive the cycle is the segment tar files QuickStart wrote
18+
# to a plain local directory (batch/hits/segments/, not process memory —
19+
# verified locally: kill -9 the JVM, restart it, the .tar.gz files are
20+
# still on disk). BENCH_DURABLE=no makes the shared driver re-run ./load
21+
# after every restart; our ./load exploits this by re-pushing those
22+
# existing tars straight to the controller's segment-upload API
23+
# (https://docs.pinot.apache.org/build-with-pinot/ingestion/batch-ingestion/segment-upload)
24+
# instead of re-parsing hits.tsv, so the "reload" on cold tries 2-129
25+
# (43 queries x 3 tries) takes seconds instead of the ~78 min a real
26+
# from-source reload would need. This keeps the real stop/start/
27+
# drop_caches cycle other systems get, rather than skipping it via
28+
# BENCH_RESTARTABLE=no.
29+
export BENCH_DURABLE=no
730
# Skip the pre-snapshot ./stop+./start cycle: the loaded
831
# state lives only in the daemon's process memory (in-process
932
# DataFrame, JVM heap caches) and stopping wipes it. The

‎pinot/check‎

Lines changed: 22 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,28 @@
11
#!/bin/bash
22
set -e
33

4-
# Pinot is responsive once both the controller and the broker accept queries.
5-
RES=$(curl -sf -o /dev/null -w '%{http_code}' \
4+
# The QuickStart process only registers its clean-shutdown hook after
5+
# ./start's launched process finishes starting every component AND
6+
# bootstrapping every sample table (including waiting for each table's
7+
# async minion segment-generation-and-push task to finish). If ./stop
8+
# sends SIGTERM any earlier than that, either shutdown-hook registration
9+
# itself throws (component startup still in flight) or an in-flight
10+
# minion task gets killed (bootstrap still in flight) -- either way the
11+
# process dies without a clean stop and corrupts the next ./start.
12+
#
13+
# Broker readiness alone is not a safe proxy for "fully done starting":
14+
# it comes up long before bootstrap finishes. The only reliable signal is
15+
# the "Quick start setup complete" line QuickStart itself prints right
16+
# after runner.bootstrapTable() + waitForBootstrapToComplete() return
17+
# (see pinot-tools Quickstart.java, method execute()). Since pinot.log is
18+
# appended across restarts, only look at what was appended after the
19+
# offset ./start recorded for this invocation.
20+
BROKER_RES=$(curl -sf -o /dev/null -w '%{http_code}' \
621
-XPOST -H'Content-Type: application/json' \
722
http://localhost:8000/query/sql/ \
8-
-d '{"sql":"SELECT 1"}')
23+
-d '{"sql":"SELECT 1"}' 2>/dev/null || echo "000")
924

10-
[ "$RES" = "200" ]
25+
OFFSET=$(cat pinot.log.offset 2>/dev/null || echo 0)
26+
SETUP_DONE=$(tail -c +"$((OFFSET + 1))" pinot.log 2>/dev/null | grep -c 'Quick start setup complete' || true)
27+
28+
[ "$BROKER_RES" = "200" ] && [ "${SETUP_DONE:-0}" -ge 1 ]

‎pinot/load‎

Lines changed: 84 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ set -e
33

44
PINOT_VERSION=1.5.1
55
PINOT_DIR="apache-pinot-$PINOT_VERSION-bin"
6+
SEGMENTS_DIR="batch/hits/segments"
67

78
# Wait for the controller's REST API to be live. `start` only waits for
89
# the broker; the controller can lag, and AddTable below talks to the
@@ -32,15 +33,68 @@ for _ in $(seq 1 60); do
3233
sleep 1
3334
done
3435

35-
# Register the schema and table. Previously trailed `|| true`, which
36-
# masked genuine failures (controller not up, bad JSON, etc.) and let
37-
# LaunchDataIngestionJob run against a missing table — the symptom
38-
# was a misleading "Failed to fetch hits/schema" deep in the ingestion
39-
# job. Fail loudly here.
40-
"./${PINOT_DIR}/bin/pinot-admin.sh" AddTable \
36+
# Register the schema and table. AddTable fails with "already exists" on
37+
# every call after the first (BENCH_DURABLE=no re-runs ./load on every
38+
# cold cycle; QuickStart's own restart wipes the table registration but
39+
# not the segment tar files on disk — see the fast path below). Treat
40+
# that specific failure as success; anything else should still fail loudly
41+
# as before.
42+
add_table_out=$("./${PINOT_DIR}/bin/pinot-admin.sh" AddTable \
4143
-tableConfigFile offline_table.json \
42-
-schemaFile schema.json -exec
44+
-schemaFile schema.json -exec 2>&1) && add_table_rc=0 || add_table_rc=$?
45+
if [ "$add_table_rc" -ne 0 ]; then
46+
if ! printf '%s\n' "$add_table_out" | grep -q 'already exists'; then
47+
printf '%s\n' "$add_table_out" >&2
48+
exit "$add_table_rc"
49+
fi
50+
fi
51+
52+
# Fast path: segment tar files from a previous ./load already exist on
53+
# disk. QuickStart -type batch writes them under $SEGMENTS_DIR (a plain
54+
# local directory, not process memory) and a stop/start cycle wipes only
55+
# the in-memory table/segment registration, not these files (verified by
56+
# reproducing this locally in Docker: kill -9 the QuickStart JVM, restart
57+
# it, curl POST each existing .tar.gz straight to the controller's
58+
# /segments endpoint — official "tar push" upload API, see
59+
# https://docs.pinot.apache.org/build-with-pinot/ingestion/batch-ingestion/segment-upload
60+
# — and the table is fully queryable again, star-tree index included,
61+
# without re-parsing hits.tsv). Re-pushing ~100 existing tars takes
62+
# seconds; re-running LaunchDataIngestionJob from hits.tsv takes ~78 min.
63+
if [ -d "$SEGMENTS_DIR" ] && ls "$SEGMENTS_DIR"/*.tar.gz >/dev/null 2>&1; then
64+
for f in "$SEGMENTS_DIR"/*.tar.gz; do
65+
curl -sS -X POST -H 'Content-Type: multipart/form-data' \
66+
-F "file=@${f}" \
67+
'http://localhost:9000/segments?tableName=hits&tableType=OFFLINE' \
68+
| grep -q 'Successfully uploaded' || {
69+
echo "bench: failed to re-push ${f}" >&2
70+
exit 1
71+
}
72+
done
73+
74+
# Re-pushed segments take a few seconds to show up in the external
75+
# view (Helix propagation), during which queries return a transient
76+
# "N segments unavailable" error. Poll until every expected segment
77+
# is ONLINE instead of racing the very first query against it
78+
# (observed locally: up to ~20s for 5 segments; sized generously
79+
# for ~100).
80+
expected=$(ls "$SEGMENTS_DIR"/*.tar.gz | wc -l | tr -d ' ')
81+
for _ in $(seq 1 120); do
82+
online=$(curl -sf http://localhost:9000/tables/hits/externalview 2>/dev/null \
83+
| python3 -c 'import json,sys
84+
d=json.load(sys.stdin)
85+
ov=d.get("OFFLINE") or {}
86+
print(sum(1 for st in ov.values() if list(st.values()) == ["ONLINE"]))' \
87+
2>/dev/null || echo 0)
88+
if [ "$online" -ge "$expected" ]; then
89+
break
90+
fi
91+
sleep 1
92+
done
4393

94+
exit 0
95+
fi
96+
97+
# Slow path (first ./load only): generate segments from hits.tsv.
4498
# Pinot was unable to load data as a single file without errors. Split.
4599
split -d --additional-suffix .tsv -n l/100 hits.tsv parts
46100

@@ -51,7 +105,30 @@ sed parts93.tsv -e 's/"tatuirovarki_redmond/tatuirovarki_redmond/g' -i
51105
sed splitted.yaml -e "s|PWD_DIR_PLACEHOLDER|$PWD|g" -i
52106
sed local.yaml -e "s|PWD_DIR_PLACEHOLDER|$PWD|g" -i
53107

108+
# pinot-admin.sh's own default is -Xms4G with NO -Xmx (falls back to the
109+
# JVM's default of ~25% of physical RAM). On a real ~74GB/100-segment
110+
# ClickBench hits.tsv this OOMed mid-job around segment 60/100 on a
111+
# 30GB-RAM c6a.4xlarge (OutOfMemoryError in SegmentDictionaryCreator) —
112+
# and LaunchDataIngestionJob still exited 0, silently leaving only 60/100
113+
# segments on disk. Force a real heap ceiling sized for this instance.
114+
export JAVA_OPTS="-Xms4G -Xmx20G"
115+
54116
"./${PINOT_DIR}/bin/pinot-admin.sh" LaunchDataIngestionJob -jobSpecFile splitted.yaml
55117

118+
# Defense against the exact failure this instance just hit: Pinot's own
119+
# ingestion job runner can swallow a per-file OutOfMemoryError (or other
120+
# per-task exception) and still exit 0, leaving fewer segment tars on
121+
# disk than input splits. Fail loudly instead of letting a partial load
122+
# silently pass bench_load's >5GB data-size check downstream.
123+
actual_segments=$(ls "$SEGMENTS_DIR"/*.tar.gz 2>/dev/null | wc -l | tr -d ' ')
124+
expected_segments=$(ls parts*.tsv 2>/dev/null | wc -l | tr -d ' ')
125+
if [ "$expected_segments" -gt 0 ] && [ "$actual_segments" -ne "$expected_segments" ]; then
126+
echo "bench: LaunchDataIngestionJob produced ${actual_segments}/${expected_segments} segments — partial load (likely OOM swallowed per-task)" >&2
127+
exit 1
128+
fi
129+
130+
# Keep hits.tsv's derived parts only as long as needed; the source file
131+
# itself and the split parts are no longer needed once segments exist in
132+
# $SEGMENTS_DIR (used by the fast path above on every later cold cycle).
56133
rm -f hits.tsv parts*.tsv
57134
sync

‎pinot/start‎

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,14 +4,30 @@ set -e
44
PINOT_VERSION=1.5.1
55
PINOT_DIR="apache-pinot-$PINOT_VERSION-bin"
66

7-
# Idempotent: if broker query endpoint is up, do nothing.
7+
# Idempotent: if broker query endpoint is up, do nothing (leave the
8+
# existing pinot.log offset marker as-is; ./check will find the earlier
9+
# "setup complete" marker in the untouched log).
810
if curl -sf -o /dev/null -w '%{http_code}' \
911
-XPOST -H'Content-Type: application/json' \
1012
http://localhost:8000/query/sql/ \
1113
-d '{"sql":"SELECT 1"}' 2>/dev/null | grep -q '^200'; then
1214
exit 0
1315
fi
1416

17+
# Record the current end of pinot.log so ./check can look only at what
18+
# this invocation appends, not a "setup complete" line left over from an
19+
# earlier run of the log-appending (>>) QuickStart process.
20+
touch pinot.log
21+
wc -c < pinot.log | tr -d ' ' > pinot.log.offset
22+
23+
# pinot-admin.sh's own default is -Xms4G with NO -Xmx (falls back to the
24+
# JVM's default of ~25% of physical RAM). On a 100-segment/~74GB ClickBench
25+
# run this OOMed mid-LaunchDataIngestionJob around segment 60/100 on a
26+
# 30GB-RAM c6a.4xlarge (observed: OutOfMemoryError in
27+
# SegmentDictionaryCreator, job silently reported success anyway).
28+
# Force a real heap ceiling sized for this instance.
29+
export JAVA_OPTS="-Xms4G -Xmx20G"
30+
1531
nohup "./${PINOT_DIR}/bin/pinot-admin.sh" QuickStart -type batch \
1632
>> pinot.log 2>&1 < /dev/null &
1733
disown

‎pinot/stop‎

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,5 +3,26 @@
33
pkill -f 'pinot-admin' 2>/dev/null || true
44
pkill -f 'pinot.tools.admin' 2>/dev/null || true
55
pkill -f 'org.apache.pinot' 2>/dev/null || true
6-
sleep 2
6+
7+
# pkill only sends SIGTERM; the actual teardown (JVM shutdown hook
8+
# stopping controller/broker/server, then the embedded Zookeeper
9+
# releasing its port) is asynchronous and takes several seconds. Two
10+
# things were previously observed to still be alive well after a fixed
11+
# short sleep or even after the broker itself stopped responding:
12+
# - the broker itself (was seen answering 200 for ~7-8s after SIGTERM)
13+
# - the embedded Zookeeper's port 2123, which a fixed sleep or a
14+
# broker-only check does not account for; the next ./start's fresh
15+
# Zookeeper can then fail to bind/connect on the still-held port,
16+
# surfacing as "ZkTimeoutException: Unable to connect to zookeeper
17+
# server within timeout" in the next start's controller.
18+
# The only reliable signal that teardown is actually finished is that
19+
# the pinot-admin JVM process itself is gone. Poll for that (up to 30s)
20+
# instead of guessing a delay or checking a single port.
21+
for _ in $(seq 1 30); do
22+
if ! pgrep -f 'org.apache.pinot.tools.admin.PinotAdministrator' > /dev/null 2>&1; then
23+
break
24+
fi
25+
sleep 1
26+
done
27+
728
exit 0

0 commit comments

Comments
 (0)