Conversation
Signed-off-by: Valery Piashchynski <piashchynski.valery@gmail.com>
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughMoves the kv plugin to v6: updates module manifests, migrates RPC proto types to v2/v6, threads context and OpenTelemetry tracing through storage operations, adds TTL conversion, updates tests, and adds package-level documentation. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant RPC as "kv RPC\n(server)"
participant Tracer as "OpenTelemetry\n(span)"
participant Storage as "Named Backend\n(memory/redis/bolt...)"
Client->>RPC: KvRequest
RPC->>Tracer: start span (ctx)
RPC->>Storage: perform operation(ctx) (Has/Set/MGet/TTL/Delete/...)
Storage-->>RPC: result (+ optional TTL duration)
RPC->>Tracer: record error? / end span
RPC-->>Client: KvResponse (items + ttl)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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.
Actionable comments posted: 2
🧹 Nitpick comments (1)
rpc.go (1)
33-44: Extract repeated storage validation/lookup into one helper.The same
storagepresence + map lookup logic is duplicated across all handlers; centralizing it will reduce drift and keep error/trace behavior consistent.Also applies to: 77-88, 106-117, 149-160, 178-189, 225-236, 260-271
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rpc.go` around lines 33 - 44, Extract the repeated "check storage name + map lookup + error recording" into a helper method (e.g., on the receiver r: func (r *RPC) resolveStorage(op string, name string, span trace.Span) (storageType, error)) that takes the operation name, the storage key (or the request), and the span, performs the in.GetStorage() empty check and r.storages map lookup, records the same span.RecordError(err) and returns a properly wrapped errors.E(op, err) on failure or the storage on success; then replace each duplicated block (the one using in.GetStorage(), errors.Str, errors.Errorf, span.RecordError, errors.E and r.storages) with a call to this helper (pass op and span) and handle its returned storage/error consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@go.mod`:
- Around line 8-9: The go.mod currently pins pre-release dependencies
github.com/roadrunner-server/api-go/v5 and
github.com/roadrunner-server/api-plugins/v5 while this module is being released
as a stable v6; do not publish a stable kv/v6 against beta v5 APIs. Fix by
reverting the module to a non-stable release (e.g., keep the module as kv/v5) or
mark this repo as a pre-release (do not publish a v6 tag) until api-go/v5 and
api-plugins/v5 reach v5.0.0 GA; specifically update go.mod and the module path
declaration that corresponds to the kv/v6 release, and ensure the require lines
for github.com/roadrunner-server/api-go/v5 and
github.com/roadrunner-server/api-plugins/v5 are either changed to stable
versions or left behind until those packages publish v5.0.0.
In `@plugin.go`:
- Around line 141-149: The function checkAndSaveStorage currently returns nil
when a requested constructor key (drStr) is missing from p.constructors, causing
silent continuation and later runtime RPC failures; change this to fail fast by
returning a descriptive error when !ok. Locate checkAndSaveStorage and replace
the nil return in the missing-constructor branch with an error value (e.g.,
fmt.Errorf or errors.Errorf) that includes the requested drStr and optionally
the list of registered keys (p.constructors) so callers see why storage
registration failed instead of silently proceeding. Ensure the error is returned
and not logged-only so upstream callers can abort.
---
Nitpick comments:
In `@rpc.go`:
- Around line 33-44: Extract the repeated "check storage name + map lookup +
error recording" into a helper method (e.g., on the receiver r: func (r *RPC)
resolveStorage(op string, name string, span trace.Span) (storageType, error))
that takes the operation name, the storage key (or the request), and the span,
performs the in.GetStorage() empty check and r.storages map lookup, records the
same span.RecordError(err) and returns a properly wrapped errors.E(op, err) on
failure or the storage on success; then replace each duplicated block (the one
using in.GetStorage(), errors.Str, errors.Errorf, span.RecordError, errors.E and
r.storages) with a call to this helper (pass op and span) and handle its
returned storage/error consistently.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
go.sumis excluded by!**/*.sumgo.work.sumis excluded by!**/*.sumtests/go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
doc.gogo.modplugin.gorpc.gotests/doc.gotests/go.modtests/kv_plugin_test.go
Signed-off-by: Valery Piashchynski <piashchynski.valery@gmail.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
plugin.go (1)
106-106: Move context creation outside the loop.
context.Background()is created inside the loop on every iteration, which is unnecessary. Since the same context is used for all storage initializations, create it once before the loop.♻️ Proposed fix
// when user requests, for example, boltdb-south, we should provide that particular pre-configured storage + ctx := context.Background() for k, v := range p.cfg.Data { // for example, if the key didn't properly format (yaml) if v == nil { continue } ... // at this point we know, that driver field present in the configuration drName := v.(map[string]any)[driver] - ctx := context.Background()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin.go` at line 106, Move the ctx := context.Background() call out of the per-storage initialization loop: create a single context before the loop that initializes storages and reuse that same ctx variable inside the loop (remove the ctx creation from inside the loop). Update the code paths that call storage initialization (the loop that references ctx in plugin.go) to use the pre-created ctx so only one Background context is created for all storage initializations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@plugin.go`:
- Line 106: Move the ctx := context.Background() call out of the per-storage
initialization loop: create a single context before the loop that initializes
storages and reuse that same ctx variable inside the loop (remove the ctx
creation from inside the loop). Update the code paths that call storage
initialization (the loop that references ctx in plugin.go) to use the
pre-created ctx so only one Background context is created for all storage
initializations.
There was a problem hiding this comment.
Pull request overview
This PR updates the KV plugin to the v6 plugin/API stack (api-go v5 kv/v2 + api-plugins v5), migrating RPC request/response types and making storage operations context-aware, along with dependency/module version bumps and small documentation additions.
Changes:
- Bump module path to
github.com/roadrunner-server/kv/v6and update dependencies toapi-go/v5+api-plugins/v5. - Migrate KV RPC handlers and tests to the new
kv/v2protobuf types (KvRequest,KvResponse,KvItem) and context-aware storage calls. - Add package docs for
kvandtests.
Reviewed changes
Copilot reviewed 8 out of 10 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
rpc.go |
Migrates RPC surface to kv/v2 protos, adds ctx-aware storage calls, and introduces TTL conversion logic. |
plugin.go |
Switches to api-plugins v5 and passes context into constructor/storage lifecycle calls. |
go.mod |
Bumps module major version to v6 and updates dependencies to api-go/api-plugins v5. |
go.sum |
Updates dependency checksums for new API modules. |
go.work.sum |
Workspace checksum updates from dependency graph changes. |
doc.go |
Adds package-level documentation for kv. |
tests/kv_plugin_test.go |
Updates integration tests to kv/v2 proto types and kv/v6 module import. |
tests/go.mod |
Updates test module dependencies + replace to kv/v6. |
tests/go.sum |
Updates test module checksums for new API modules. |
tests/doc.go |
Adds package-level documentation for integration tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Valery Piashchynski <piashchynski.valery@gmail.com>
Signed-off-by: Valery Piashchynski <piashchynski.valery@gmail.com>
Signed-off-by: Valery Piashchynski <piashchynski.valery@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/go.mod (1)
8-18:⚠️ Potential issue | 🔴 CriticalThe integration harness requires v6-major storage drivers, but none are yet released.
plugin.gonow usesgit.832008.xyz/roadrunner-server/api-plugins/v6/kv.Constructor, which requires compatible v6-major drivers. However,tests/go.modstill importsboltdb/v5,memcached/v5,memory/v5,redis/v5, andrpc/v5. The v6 KV API contract cannot be satisfied by v5 drivers, leavingp.constructorsempty at runtime. This matches the CI failure (no such constructor was registered: boltdb, registered: map[]).As of now, v6 releases do not exist for any of these storage drivers—only v5.x is published. The test must either wait for
boltdb/v6,memcached/v6,memory/v6, andredis/v6releases, or the KV plugin versioning must be reconsidered to match currently available driver versions.
♻️ Duplicate comments (1)
go.mod (1)
8-9:⚠️ Potential issue | 🟠 MajorPrerelease API modules still block a GA
kv/v6cut.PR
#107is preparing a stable v6 release, but Lines 8-9 still pin the core API packages tov6.0.0-beta.1. Cuttingkv/v6as GA while its primary API contracts are still prerelease keeps downstream compatibility unstable.Are `github.com/roadrunner-server/api-go/v6` and `github.com/roadrunner-server/api-plugins/v6` published as stable `v6.0.0` releases yet, or are the latest available v6 tags still prereleases?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@go.mod` around lines 8 - 9, The go.mod pins to prerelease tags for github.com/roadrunner-server/api-go/v6 and github.com/roadrunner-server/api-plugins/v6 which blocks cutting a GA kv/v6; verify the upstream tags for those two modules and update the two entries in go.mod to stable v6.0.0 if those v6 tags are published, otherwise revert them to the latest published stable v5 tags (or add a temporary replace directive to a specific commit/branch) so the module does not depend on beta releases; ensure you update the versions for the exact module paths github.com/roadrunner-server/api-go/v6 and github.com/roadrunner-server/api-plugins/v6 accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/linux.yml:
- Line 54: The go test invocation missing the coverage package selector causes
tests run under tests/... to not record coverage for the KV module; update the
go test command (the line containing "go test -timeout 20m -v -race -cover
-tags=debug -failfast -coverprofile=./coverage-ci/kv.out -covermode=atomic
./...") to include the flag -coverpkg=github.com/roadrunner-server/kv/v6/... so
the generated coverage includes the KV package (which the downstream AWK filter
that matches "github.com/roadrunner-server/kv/v6/" expects).
In `@rpc.go`:
- Around line 205-210: The code currently treats malformed non-empty TTL strings
as absent by leaving item.Ttl nil; change the logic in rpc.go where you build
kvv2.KvItem so that when ret[k] != "" and time.Parse(time.RFC3339, ret[k])
returns an error you return a failed RPC (e.g., propagate an error using the RPC
error path such as status.Errorf with an appropriate codes.InvalidArgument or
codes.Internal), rather than silently ignoring the parse error; keep the
successful path that sets item.Ttl = durationpb.New(time.Until(t)) when parse
succeeds.
---
Duplicate comments:
In `@go.mod`:
- Around line 8-9: The go.mod pins to prerelease tags for
github.com/roadrunner-server/api-go/v6 and
github.com/roadrunner-server/api-plugins/v6 which blocks cutting a GA kv/v6;
verify the upstream tags for those two modules and update the two entries in
go.mod to stable v6.0.0 if those v6 tags are published, otherwise revert them to
the latest published stable v5 tags (or add a temporary replace directive to a
specific commit/branch) so the module does not depend on beta releases; ensure
you update the versions for the exact module paths
github.com/roadrunner-server/api-go/v6 and
github.com/roadrunner-server/api-plugins/v6 accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a175a90a-a5f4-4642-8be6-35a9a373b527
⛔ Files ignored due to path filters (3)
go.sumis excluded by!**/*.sumgo.work.sumis excluded by!**/*.sumtests/go.sumis excluded by!**/*.sum
📒 Files selected for processing (8)
.github/workflows/linux.yml.golangci.ymlgo.modplugin.gorpc.gotests/go.modtests/kv_plugin_test.gotests/pkgs.txt
💤 Files with no reviewable changes (2)
- tests/pkgs.txt
- .golangci.yml
Signed-off-by: Valery Piashchynski <piashchynski.valery@gmail.com>
Signed-off-by: Valery Piashchynski <piashchynski.valery@gmail.com>
Reason for This PR
Description of Changes
License Acceptance
By submitting this pull request, I confirm that my contribution is made under the terms of the MIT license.
PR Checklist
[Author TODO: Meet these criteria.][Reviewer TODO: Verify that these criteria are met. Request changes if not]git commit -s).CHANGELOG.md.Summary by CodeRabbit
New Features
Chores
Tests & Documentation