mrproliu opened a new pull request, #1220:
URL: https://github.com/apache/skywalking-banyandb/pull/1220
## What this fixes
The liaison's periodic `top bydbql cache-miss queries` log (added in #1213,
controlled by `--bydbql-topk-log-interval`) exists to surface query templates
that **thrash** the prepared-statement cache — templates evicted and
re-parsed
over and over. On a healthy cluster it instead reports a large list of
fabricated entries, making it useless for the diagnosis it was built for.
## The bug
A cache *miss* is not the same as *thrashing*. Every parameterized template
pays one unavoidable **first-ever compile** — a single miss — and on a
cluster
whose cache is large enough that nothing is evicted, that is the *only* miss
it
ever has. The tracker was fed **every** miss, cold starts included.
That breaks the tracker in two compounding ways:
1. Cold-start compiles bury any real thrashing signal.
2. The tracker is a bounded Space-Saving heavy-hitter with `bydbqlTopKSize =
128`
slots. Once the number of distinct templates exceeds that, eviction kicks
in,
and a new key **inherits the evicted minimum's `count + 1`**. With `N`
observations over `k` slots, every reported count ratchets to roughly
`N/k` —
pure artifact, not real frequency. The `minReparseMisses = 2` filter, the
only
thing then separating thrashing from cold starts, is silently defeated.
### Measured on a live SkyWalking OAP cluster
2495 distinct parameterized templates against a cache large enough to hold
them
all — **nothing is ever evicted, so every true miss count is exactly 1**:
| | before | after |
|---|---|---|
| entries logged as "thrashing" | **128** (all 128 slots full) | **0** |
| reported counts | 19–20 (`2495/128 ≈ 19.5`) | n/a (empty) |
| true re-parse count | **1** for every template | — |
A completely healthy cluster reported 128 templates as thrashing, with counts
no query ever actually reached.
## The fix
Classify at the source rather than guessing from miss counts. The prepared
cache already has an eviction callback (it adjusts the byte accounting); it
now
also records each evicted key so a later miss on one can be recognised as a
**re-parse** instead of a first-ever compile:
- Evicted keys are remembered by **xxhash** (`cespare/xxhash/v2`, already a
direct dependency) — 8 bytes each, ~16 KB for a 2000-entry cache. Storing
the
full query text instead would duplicate multi-KB text, escape the
`--bydbql-prepared-cache-max-bytes` bound, and pin the memory of queries
the
LRU just evicted.
- The classification is done inside the cache, **before** `store()` runs,
because
`store()`'s own eviction would otherwise displace the evidence. A
statement too
large to ever cache (`cost > maxBytes`) is also reported as a re-parse —
it is
re-parsed on every request and the evicted set can never witness it.
- `getOrPrepare` returns `"reparse"` for these; only re-parses reach the
tracker,
and first-ever compiles never do.
With cold starts excluded at the source, the `count >= 2` filter is dropped:
it
existed only to hide cold starts, and keeping it would now re-hide genuine
single re-parses (leaving the log *less* sensitive than before). The tracker
stays empty on a healthy cluster, and its counts are exact when thrashing is
real.
- [ ] If this pull request closes/resolves/fixes an existing issue, replace
the issue number. Fixes apache/skywalking#<issue number>.
- [ ] Update the [`CHANGES`
log](https://github.com/apache/skywalking-banyandb/blob/main/CHANGES.md).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
