Repository navigation
feat(stream): give each tenant its own keepalive wheel - #738
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (19)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📜 Recent review details🧰 Additional context used📚 Code guidelines (4)📓 Path-based instructions (5)Source excerpt: In a Docker / Podman / Kubernetes deployment, **`data_dir` must resolve to a host-backed volume**.📄 CodeRabbit inference engine (docs/src/content/docs/deployment.md) Files:
Source excerpt: Create `*_test.go` files in the same package as the code under test.📄 CodeRabbit inference engine (AGENTS.md) Files:
See [AGENTS.md](../AGENTS.md) for project conventions, architecture notes, and AI agent instructions.📄 CodeRabbit inference engine (.github/copilot-instructions.md) Files:
Source excerpt: **In MDX, leave a blank line between a JSX tag and a code fence.**📄 CodeRabbit inference engine (AGENTS.md) Files:
Source excerpt: **Docs prose**: never hard-wrap Markdown — one paragraph is one line.📄 CodeRabbit inference engine (CONTRIBUTING.md) Files:
🪛 LanguageTooldocs/src/content/docs/configuration.mdx[style] ~140-~140: Since ownership is already implied, this phrasing may be redundant. (PRP_OWN) [style] ~140-~140: Since ownership is already implied, this phrasing may be redundant. (PRP_OWN) docs/src/content/docs/architecture.md[style] ~95-~95: This phrase is redundant. Consider writing “last”. (LAST_OF_ALL) [style] ~95-~95: Since ownership is already implied, this phrasing may be redundant. (PRP_OWN) docs/src/content/docs/deployment.md[style] ~574-~574: Since ownership is already implied, this phrasing may be redundant. (PRP_OWN) [style] ~574-~574: Since ownership is already implied, this phrasing may be redundant. (PRP_OWN) [style] ~574-~574: Since ownership is already implied, this phrasing may be redundant. (PRP_OWN) [style] ~574-~574: Since ownership is already implied, this phrasing may be redundant. (PRP_OWN) [style] ~574-~574: Since ownership is already implied, this phrasing may be redundant. (PRP_OWN) [style] ~574-~574: Since ownership is already implied, this phrasing may be redundant. (PRP_OWN) [style] ~574-~574: Since ownership is already implied, this phrasing may be redundant. (PRP_OWN) CHANGELOG.md[typographical] ~13-~13: Consider using an em dash in dialogues and enumerations. (DASH_RULE) [style] ~13-~13: This sentence is over 40 words long. Consider splitting it up, as shorter sentences make the text easier to read. (TOO_LONG_SENTENCE) [style] ~13-~13: Since ownership is already implied, this phrasing may be redundant. (PRP_OWN) [style] ~13-~13: Consider shortening this phrase to just ‘whether’, unless you mean ‘regardless of whether’. (WHETHER) 🔇 Additional comments (19)
📝 SummarySummary by CodeRabbit
WalkthroughThe app now maintains a separate SSE keepalive wheel for each served tenant. Stream handlers select the wheel using the request tenant’s settings. Adoption, settings reloads, tenant removal, and shutdown update the corresponding wheels. ChangesTenant-scoped SSE keepalive behavior
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix · Severity of issue fixed: Medium Sequence Diagram(s)sequenceDiagram
participant TenantSettings
participant AfterAdopt
participant keepalives
participant StreamHandler
participant Heartbeater
TenantSettings->>AfterAdopt: Adopt tenant settings
AfterAdopt->>keepalives: Reconcile tenant wheels
StreamHandler->>keepalives: Resolve wheel for request tenant
keepalives-->>StreamHandler: Return tenant wheel or nil
StreamHandler->>Heartbeater: Register subscriber when a wheel is returned
Suggested reviewers: Merge Risk: ⚪ Minimal · up to No material issue remains established for the tenant-specific keepalive change; it is mergeable after normal checks. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
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 |
github-actions
Bot
added
documentation
|
📚 Docs preview is live → https://efd35efb-wavehouse-docs.wave-rf.workers.dev |
Code Coverage OverviewLanguages: Go GoThe overall line coverage in commit 056e7a2 in the Show a line coverage summary of the most impacted files.
Updated |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
056e7a2
2 participants
Summary
stream.keepalive_intervalandstream.keepalive_bucketsare per tenant, but one keepalive wheel served the whole process at the shortest interval among the served tenants, so one tenant's setting, or its reload, set every tenant's cadence. Each served tenant now has a wheel of its own.internal/app/keepalives.gokeeps onestream.Heartbeaterper served tenant, reconciled fromAfterAdoptthe waydiscoveriesis: built from the tenant's ownstore.Keepalive(), reshaped in place on that tenant's reload, stopped and dropped when the tenant is removed or rejected.Heartbeateris a lookup by the request's store. A tenant with no wheel is no longer served and its stream is already ending, so it goes without one. A lookup that lands after a reload adopted a tenant and before the keepalive hook ran reconciles first, so a served tenant always has a wheel.keepalivecomponent, so underRun's errgroup as the one wheel did.shortestKeepaliveis deleted. A flat directory behaves as before.A wheel runs from its tenant's adoption, not from its first stream. Idle, it is one parked goroutine (under 5 KB) and a wake of about 3 µs every
keepalive_interval ÷ keepalive_buckets, ten seconds at the defaults. A thousand idle tenants cost about 5 MB and 0.03% of one core, less than a start and stop racing every stream's open and close would be worth.Test plan
synctestclock: two tenants at 30 and 10 seconds are each nudged at their own interval; one tenant's reload to 1 second leaves the other's next keepalive where it was due; a tenant's own reload reshapes its wheel in place with its stream still on itRunGET /v1/streamconnections over a nested directory, read off the keepalive commentsmake ciRelated Issues
Closes #597
Part of #583
Follow-ups
stream.keepalive_bucketshas no upper bound, so a tenant can ask for a ring, and a tick rate, of any size. It is per tenant now.TestExternalNATS_AcksUnderBothAckSubjectLayouts(internal/mq, from feat(mq): external nats broker with sharded, pinned ingest workers #624) is flaky: it failed two of three localmake ciruns withcontext canceled. Not touched here.internal/app's unit tests run close to the 15-second budget (test(app): internal/app unit tests use 8–17 s of the 15 s budget, time out under load #617, test(mq,app): fit the unit budget; fail fast on an uncreatable store #647); this adds about 0.3 seconds.