Cache broadcast live query bitfields in memory#48047
Conversation
|
@coderabbitai full review |
✅ Action performedFull review finished. |
WalkthroughThe change reduces Redis load from concurrent live queries targeting small host sets by introducing an in-memory bitfield cache for broadcast query targeting. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/live_query/redis_live_query.go (1)
379-388:⚠️ Potential issue | 🟠 Major | ⚡ Quick winLoad the cache before checking out the reverse-query Redis connection.
Line 380 acquires a Redis connection before Line 387 can enter
ensureCacheLoaded; if the cache is expired, waiters can hold pool connections while the singleflight leader needs additional Redis connections inloadCache, risking pool starvation during check-in bursts. Move the cache load ahead ofReadOnlyConn.Proposed fix
func (r *redisLiveQuery) collectReverseQueriesForHost(hostID uint, queriesByHost map[string]string) error { - conn := redis.ReadOnlyConn(r.pool, r.pool.Get()) - defer conn.Close() - // Stale-entry filtering below relies on the SQL cache holding only active // queries. Refresh it on expiry here so this path stays correct on its own, // independent of any cache (re)load done by the caller or the bitfield path // (which is skipped when every active query is small-target). if err := r.ensureCacheLoaded(); err != nil { return fmt.Errorf("load cache: %w", err) } + + conn := redis.ReadOnlyConn(r.pool, r.pool.Get()) + defer conn.Close() names, err := redigo.Strings(conn.Do("SMEMBERS", reverseHostKey(hostID)))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/live_query/redis_live_query.go` around lines 379 - 388, The Redis connection is being acquired via redis.ReadOnlyConn before the cache is loaded via ensureCacheLoaded in the collectReverseQueriesForHost function. This creates a risk of pool starvation if the cache is expired and other waiters are holding connections while the singleflight leader needs additional connections in loadCache. Move the ensureCacheLoaded call to execute at the start of the function, before the Redis connection acquisition, so the cache is guaranteed to be loaded before any pool connections are checked out.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@server/live_query/redis_live_query.go`:
- Around line 379-388: The Redis connection is being acquired via
redis.ReadOnlyConn before the cache is loaded via ensureCacheLoaded in the
collectReverseQueriesForHost function. This creates a risk of pool starvation if
the cache is expired and other waiters are holding connections while the
singleflight leader needs additional connections in loadCache. Move the
ensureCacheLoaded call to execute at the start of the function, before the Redis
connection acquisition, so the cache is guaranteed to be loaded before any pool
connections are checked out.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 086ab503-f5d7-436a-8f7c-7dbbdb00f99f
📒 Files selected for processing (3)
changes/42441-live-query-redis-scalingserver/live_query/redis_live_query.goserver/live_query/redis_live_query_test.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 42441-live-query-redis-re-architecture-set-on-small-liveq #48047 +/- ##
==========================================================================================
Coverage 67.30% 67.31%
==========================================================================================
Files 3655 3655
Lines 231336 231393 +57
Branches 12075 12075
==========================================================================================
+ Hits 155711 155762 +51
- Misses 61642 61645 +3
- Partials 13983 13986 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Relates to #42441 Cache each active broadcast query's targeting bitfield in the existing in-memory cache and test the host's bit locally on check-in, so a broadcast query no longer costs a Redis command per check-in. A query whose bitfield is not cached (e.g. it expired while the campaign is still active) falls back to a live GETBIT.
juan-fdz-hawa
force-pushed
the
42441-live-query-redis-re-architecture-cache-bimap
branch
from
311bb94 to
dd8e213
Compare
1 participant
Related issue: Relates to #42441
Cache each active broadcast query's targeting bitfield in the existing in-memory cache and test the host's bit locally on check-in, so a broadcast query no longer costs a Redis command per check-in. A query whose bitfield is not cached (e.g. it expired while the campaign is still active) falls back to a live GETBIT.
Checklist for submitter
If some of the following don't apply, delete the relevant line.
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
Testing
Summary by CodeRabbit
Bug Fixes
Tests