mrproliu opened a new pull request, #1221:
URL: https://github.com/apache/skywalking-banyandb/pull/1221
## What
Two related changes to how a liaison reports and survives an unreachable
data node:
1. **Expire BydbQL top-K entries** so the periodic log describes the present
instead of everything since process start.
2. **Enable the periodic health check on the queue client**, so a data node
that dies is evicted by a background probe instead of by a query failing on it.
They ship together because the first is what made the second visible: a
slow-query log that never forgets is how a 20-hour-old startup incident kept
being reported as a current problem.
## Why
A production cluster reported eight slow BydbQL queries, the worst at
15.003s. Investigating them turned up two separate defects.
### The report never expires
The top-K trackers accumulate for the life of the process, and `max_latency`
is a running peak. Twenty-one hours after a one-off startup incident, the log
still read:
```
"SELECT ... FROM MEASURE service_traffic_minute IN sw_metadata ..." count=2
max_latency=15.003149126s
```
Zero recurrences in those 21 hours. Anyone reading that log would conclude
the cluster was currently unhealthy, and nothing in the output distinguishes a
live problem from a stale peak.
### The routing table never self-corrects
`banyand/queue/pub` builds its `ConnManager` without `HealthCheckInterval`.
The zero value means `periodicHealthCheck` never starts, so the active set is
validated only on admission and, after that, only when a request happens to
fail on a node. A node that dies stays routable until some query picks it and
pays that query's full timeout to discover it.
The same process shows the contrast during one rolling restart — same node,
same moment:
```
SERVER-QUEUE-PUB-DATA (query path)
07:55:46 new node is healthy, add it to active queue
── 51 seconds, zero health checks ──
07:56:38 healthCheck: service unhealthy ← first re-check, after a
query had already failed
07:56:38 moving to evictable
PROPERTY-SCHEMA-REGISTRY (sets HealthCheckInterval=10s)
07:56:06 / 07:56:09 / 07:56:13 / 07:56:19 / 07:56:29 ← five checks,
evicted promptly
```
The seven `sw_metadata` slow queries were issued between 07:56:20 and
07:56:29 (derived from `error timestamp − latency`), entirely inside that
51-second blind spot. Each failed at stream creation with `DeadlineExceeded`
after burning its query type's full budget:
```
"error": "... failed to get stream for node demo-banyandb-data-hot-0...:
rpc error: code = DeadlineExceeded desc = context deadline
exceeded",
"latency": 15001.082906
```
Two queries issued at 07:56:37, after the node was finally evicted, failed
in **0.6s** instead of 15s — the same restart, the same node, a 20× difference
once the routing table was correct.
## Changes
### BydbQL top-K TTL
- `--bydbql-topk-slow-ttl` / `--bydbql-topk-reparse-ttl` (both default
`24h`, `0` restores the previous cumulative behaviour). An entry whose query
has not recurred within its TTL is dropped.
- Every logged entry now carries `last_seen`; slow entries also carry
`max_latency_at`, dating the peak for queries that do keep recurring.
- When a tracker is full, expired entries are reclaimed before a live entry
is evicted, so a stale-but-frequent entry cannot crowd out a new one.
- Expiry runs in `copyOut()`, not only in `observe()`. An entry nobody
observes again would otherwise never be revisited and would be reported forever
— which is precisely the case the TTL exists for.
### Queue client periodic health check
- `--<prefix>-client-health-check-interval` (default `10s`, `0` disables) on
the `data`/`liaison` queue clients, wired into `ConnManagerConfig`.
- `ConnManager` has always implemented the prober and `pkg/grpchelper`
already tests it; the queue client simply never passed an interval. This
connects existing machinery rather than adding any.
- `NewWithoutMetadata` deliberately stays at zero: it skips `FlagSet`, and
its callers (lifecycle migration, tests) are short-lived and do not all reach
`GracefulStop`, so a prober they never stop would leak a goroutine.
## Testing
- 5 new TTL cases covering expiry, refresh-on-observe, `ttl<=0`,
reclaim-before-evict, and `maxDurAt` tracking the peak rather than the latest
observation. All use the repo's `timestamp.NewMockClock()`; none sleep.
- 2 new wiring tests for the health check, driving the real `PreRun` path
rather than the `initConnMgr` test helper that bypasses it. The behavioural one
asserts a node is evicted **without any request being sent to it** — only a
background prober can do that.
- 1 new wiring test for `newTopKDumper`, whose two TTLs are adjacent
`time.Duration` parameters where a swap would be silent.
Each wiring test was mutation-checked: removing the line it guards makes it
fail, and the health-check one fails the same way the production bug manifested
(node never evicted).
`make lint` passes across all ten projects; `go test ./banyand/liaison/...
./banyand/queue/...` is green.
## Notes for reviewers
- **`snapshot()` is no longer cumulative since process start.** Counts now
accumulate over an entry's lifetime, which the TTL bounds. Comments claiming
otherwise were corrected.
- **A related fix was considered and deliberately deferred.** Read queries
could pass `grpc.WaitForReady(false)` so an unreachable node fails in
milliseconds instead of parking until the deadline. It addresses the amplifier
rather than the root cause, and it would turn "a 2-second blip the query
survives" into an immediate failure. Worth revisiting once the periodic check
is deployed and the residual exposure can be measured.
- **Not addressed here:** BydbQL counters are lazily registered, so their
series are born non-zero and `increase()` under-reports after a restart; BydbQL
has no latency histogram; and PROPERTY/STREAM/TRACE distributed query timeouts
remain hardcoded while only MEASURE has `--dst-broadcast-timeout`.
- [ ] If this pull request closes/resolves/fixes an existing issue, replace
the issue number. Fixes apache/skywalking#<issue number>.
- [x] 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]
