🐛 fix grpc benchmark #3342
🐛 fix grpc benchmark #3342
Conversation
📝 WalkthroughWalkthroughRemoved explicit context parameters from benchmark Operation and Starter Run signatures; call sites now obtain context via testing.B/TB (b.Context()/tb.Context()). Replaced manual sync.WaitGroup goroutine coordination with errgroup-based concurrency and adjusted streaming EOF/status/error handling. Changes
Sequence Diagram(s)sequenceDiagram
participant TB as testing.TB / testing.B
participant Runner as Starter.Run(tb)
participant Eg as errgroup
participant Worker as goroutine (worker)
TB->>Runner: Run(tb) -> stop()
Runner->>Eg: eg, ctx := errgroup.New(tb.Context())
Note right of Eg: spawn workers via eg.Go(...)
Eg->>Worker: eg.Go(worker)
Worker-->>Eg: return error / nil
Runner->>Eg: eg.Wait() on stop()
Eg-->>Runner: aggregated error / nil
sequenceDiagram
participant B as testing.B
participant Streamer as StreamInsert/StreamSearch
participant Eg as errgroup
participant Sender as sender goroutine (wg)
participant Receiver as recv goroutine (eg.Go)
B->>Streamer: StreamX(b, ...)
Streamer->>Eg: eg, ctx := errgroup.New(b.Context())
Streamer->>Sender: start send loop (wg)
Streamer->>Receiver: eg.Go(recv loop)
Receiver-->>Eg: io.EOF -> return nil
Receiver-->>Eg: other error -> return err
Streamer->>Eg: eg.Wait() & wg.Wait()
Eg-->>Streamer: propagate errors
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 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 |
|
[CHATOPS:HELP] ChatOps commands.
|
github-actions
bot
added
actions/backport/release/v1.7
actions/build/amd64
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
hack/benchmark/internal/starter/agent/core/ngt/ngt.go (1)
49-93: errgroup migration inRunlooks correct; context layering could be simplifiedThe switch to a local
errgroup.New(ctx)and joining viaeg.Go/eg.Wait()preserves the previous WaitGroup semantics and looks correct for this starter. Both goroutines always returnnil, so ignoringeg.Wait()’s error is harmless with the current code.If you want to simplify the context chain slightly, you could consider creating the cancelable context first and then passing it into
errgroup.New, so there’s only a single derived context instead ofctx (arg) -> errgroup ctx -> WithCancel ctx. Not required, but it might make the flow a bit easier to follow.hack/benchmark/internal/operation/search.go (1)
133-191: Redundantsc != nilchecks in StreamSearchByID loop conditionsIn
StreamSearchByID,scis created once and only checked, never reassigned tonil, so:
- The recv loop condition
for sc != nil { ... }and- The sender loop guard
&& sc != nilare effectively always true given the current code. Termination is already controlled via EOF (setting
finishedto true) and the benchmark’s context.You could simplify this by dropping the
sc != nilchecks (e.g.,for { ... }andfor i := 0; i < b.N && !finished.Load(); i++), which would make the control flow a bit clearer without changing behavior.hack/benchmark/internal/operation/insert.go (1)
66-135: Consider loggingeg.Wait()errors in StreamInsertIn
StreamInsert, the recv goroutine now returns a non-nilerror on non-EOF failures:if err != nil { // ... return err }This is good for triggering errgroup’s context cancellation and shutting down the stream early. However, the subsequent
eg.Wait()call:eg.Wait()ignores that error, so any Recv error that doesn’t also show up on
Send/CloseSendwill be invisible at the benchmark level.If you want better visibility into such failures, you could optionally log the result of
eg.Wait():- eg.Wait() + if err := eg.Wait(); err != nil { + grpcError(b, err) + }This keeps the early-cancel behavior while making hidden Recv-side errors easier to diagnose.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
hack/benchmark/internal/operation/insert.go(5 hunks)hack/benchmark/internal/operation/remove.go(5 hunks)hack/benchmark/internal/operation/search.go(5 hunks)hack/benchmark/internal/starter/agent/core/ngt/ngt.go(4 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: kpango
Repo: vdaas/vald PR: 2491
File: pkg/gateway/lb/handler/grpc/handler.go:306-316
Timestamp: 2024-10-08T20:16:00.642Z
Learning: User: kpango
PR: vdaas/vald#2491
File: pkg/gateway/lb/handler/grpc/handler.go:306-316
Timestamp: 2024-05-07T04:15:11.886Z
Learning: In this project, vtprotobuf is used, which provides enhanced methods for protobuf objects: CloneVT (object cloning), EqualVT (object comparison), SizeVT (returns object size), MarshalVT (faster version of proto.Marshal), and UnmarshalVT (faster version of proto.Unmarshal). These methods offer better performance than usual protobuf usage.
Learnt from: kpango
Repo: vdaas/vald PR: 2491
File: pkg/gateway/lb/handler/grpc/handler.go:306-316
Timestamp: 2024-06-10T19:25:49.832Z
Learning: User: kpango
PR: vdaas/vald#2491
File: pkg/gateway/lb/handler/grpc/handler.go:306-316
Timestamp: 2024-05-07T04:15:11.886Z
Learning: In this project, vtprotobuf is used, which provides enhanced methods for protobuf objects: CloneVT (object cloning), EqualVT (object comparison), SizeVT (returns object size), MarshalVT (faster version of proto.Marshal), and UnmarshalVT (faster version of proto.Unmarshal). These methods offer better performance than usual protobuf usage.
Learnt from: kpango
Repo: vdaas/vald PR: 0
File: :0-0
Timestamp: 2025-05-01T08:44:07.726Z
Learning: PR #2927 addressed gRPC connection panic issues by removing the singleflight.Group[pool.Conn] field from the gRPCClient struct in internal/net/grpc/client.go and introducing a centralized handleError function for consistent error processing in both Connect and Disconnect methods.
Learnt from: kpango
Repo: vdaas/vald PR: 0
File: :0-0
Timestamp: 2025-05-01T08:44:07.726Z
Learning: PR #2927 addressed gRPC connection panic issues by removing the singleflight.Group[pool.Conn] field from the gRPCClient struct in internal/net/grpc/client.go and introducing a centralized handleError function for more consistent error processing in both Connect and Disconnect methods.
Learnt from: kpango
Repo: vdaas/vald PR: 0
File: :0-0
Timestamp: 2025-05-01T08:44:07.726Z
Learning: PR #2927 removed the singleflight.Group field from the gRPCClient struct in internal/net/grpc/client.go and introduced a centralized handleError function for error processing to address gRPC connection panic issues.
Learnt from: Matts966
Repo: vdaas/vald PR: 2928
File: pkg/gateway/lb/handler/grpc/linear_search.go:271-333
Timestamp: 2025-04-15T05:32:26.428Z
Learning: For the vald project, refactoring PRs should be kept separate from semantic changes that alter behavior. The current PR "Divide LB Gateway handler.go" is focused solely on splitting the file into smaller, more manageable components without changing functionality.
Learnt from: kpango
Repo: vdaas/vald PR: 2491
File: pkg/gateway/lb/handler/grpc/aggregation.go:500-505
Timestamp: 2024-06-10T19:25:49.832Z
Learning: User: kpango
PR: vdaas/vald#2491
File: pkg/gateway/lb/handler/grpc/aggregation.go:500-505
Timestamp: 2024-05-08T02:17:42.078Z
Learning: The `GetNum()` method in the `valdStdAggr` struct should include a nil check to prevent runtime panics when called on a nil instance. This is a defensive programming practice to enhance the robustness of the code.
📚 Learning: 2025-05-01T08:44:07.726Z
Learnt from: kpango
Repo: vdaas/vald PR: 0
File: :0-0
Timestamp: 2025-05-01T08:44:07.726Z
Learning: PR #2927 addressed gRPC connection panic issues by removing the singleflight.Group[pool.Conn] field from the gRPCClient struct in internal/net/grpc/client.go and introducing a centralized handleError function for more consistent error processing in both Connect and Disconnect methods.
Applied to files:
hack/benchmark/internal/operation/insert.gohack/benchmark/internal/operation/remove.gohack/benchmark/internal/starter/agent/core/ngt/ngt.gohack/benchmark/internal/operation/search.go
📚 Learning: 2025-05-01T08:44:07.726Z
Learnt from: kpango
Repo: vdaas/vald PR: 0
File: :0-0
Timestamp: 2025-05-01T08:44:07.726Z
Learning: PR #2927 addressed gRPC connection panic issues by removing the singleflight.Group[pool.Conn] field from the gRPCClient struct in internal/net/grpc/client.go and introducing a centralized handleError function for consistent error processing in both Connect and Disconnect methods.
Applied to files:
hack/benchmark/internal/operation/insert.gohack/benchmark/internal/operation/remove.gohack/benchmark/internal/starter/agent/core/ngt/ngt.gohack/benchmark/internal/operation/search.go
📚 Learning: 2025-05-01T08:44:07.726Z
Learnt from: kpango
Repo: vdaas/vald PR: 0
File: :0-0
Timestamp: 2025-05-01T08:44:07.726Z
Learning: PR #2927 removed the singleflight.Group field from the gRPCClient struct in internal/net/grpc/client.go and introduced a centralized handleError function for error processing to address gRPC connection panic issues.
Applied to files:
hack/benchmark/internal/operation/insert.gohack/benchmark/internal/operation/remove.gohack/benchmark/internal/starter/agent/core/ngt/ngt.gohack/benchmark/internal/operation/search.go
📚 Learning: 2024-06-10T19:25:49.832Z
Learnt from: kpango
Repo: vdaas/vald PR: 2491
File: pkg/gateway/lb/handler/grpc/handler.go:306-316
Timestamp: 2024-06-10T19:25:49.832Z
Learning: User: kpango
PR: vdaas/vald#2491
File: pkg/gateway/lb/handler/grpc/handler.go:306-316
Timestamp: 2024-05-07T04:15:11.886Z
Learning: In this project, vtprotobuf is used, which provides enhanced methods for protobuf objects: CloneVT (object cloning), EqualVT (object comparison), SizeVT (returns object size), MarshalVT (faster version of proto.Marshal), and UnmarshalVT (faster version of proto.Unmarshal). These methods offer better performance than usual protobuf usage.
Applied to files:
hack/benchmark/internal/operation/remove.gohack/benchmark/internal/starter/agent/core/ngt/ngt.gohack/benchmark/internal/operation/search.go
🧬 Code graph analysis (4)
hack/benchmark/internal/operation/insert.go (1)
internal/sync/errgroup/group.go (1)
Wait(253-258)
hack/benchmark/internal/operation/remove.go (2)
internal/io/io.go (1)
EOF(40-40)internal/sync/errgroup/group.go (1)
Wait(253-258)
hack/benchmark/internal/starter/agent/core/ngt/ngt.go (1)
internal/sync/errgroup/group.go (1)
Wait(253-258)
hack/benchmark/internal/operation/search.go (1)
internal/sync/errgroup/group.go (1)
Wait(253-258)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: build / build (amd64, ubuntu-latest)
- GitHub Check: grpc-stream
- GitHub Check: grpc-sequential
- GitHub Check: build / build (amd64, ubuntu-latest)
- GitHub Check: CodeQL Analyze (go)
- GitHub Check: CodeQL Analyze (actions)
- GitHub Check: CodeQL Analyze (rust)
- GitHub Check: runner / go build
- GitHub Check: check-format-diff
- GitHub Check: runner / LanguageTool / apis & charts
- GitHub Check: runner / textlint
- GitHub Check: Cloudflare Pages
🔇 Additional comments (2)
hack/benchmark/internal/operation/search.go (1)
55-110: StreamSearch errgroup refactor is consistent and preserves behaviorUsing
ctx := b.Context()pluseg, ctx := errgroup.New(ctx)and moving the recv loop intoeg.Gowhile waiting witheg.Wait()keeps the previous behavior (logging viagrpcErrorand looping until EOF) and just swaps out the WaitGroup for errgroup. Since the goroutine always returnsnil, ignoringeg.Wait()here is fine.hack/benchmark/internal/operation/remove.go (1)
51-107: StreamRemove errgroup conversion looks goodThe migration from a local WaitGroup to
errgroup.New(ctx)inStreamRemoveis straightforward:
ctx := b.Context()ensures the stream is tied to the benchmark lifecycle.- The recv loop correctly exits on
io.EOFand keeps logging other errors viagrpcError.eg.Wait()is only joining a goroutine that always returnsnil, so ignoring its result matches the previous WaitGroup semantics.No issues spotted here.
Deploying vald with
|
| Latest commit: |
35b1c27
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://7d22c0bc.vald.pages.dev |
| Branch Preview URL: | https://bugfix-benchmark-grpc.vald.pages.dev |
kmrmt
requested review from
a team,
Matts966 and
kpango
and removed request for
a team
kmrmt
dismissed stale reviews from Matts966 and coderabbitai[bot]
via
768404a
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
hack/benchmark/internal/starter/agent/core/ngt/ngt.go (1)
78-87: Mixing localegwith globalerrgroup.Gopreventseg.Wait()from waiting on the event loopHere you start the
echlistener with the package-levelerrgroup.Go, but the cleanup waits on the localeg:eg, ctx := errgroup.New(ctx) ... errgroup.Go(func() error { ... }) ... eg.Wait()Because
errgroup.Goattaches the goroutine to the default group (seeinternal/sync/errgroup/group.go),eg.Wait()will never wait for this listener, and any error it returns will be tracked on the global group instead of the local one. That both undermines the “use a local instance of errgroup.Group” goal and can make listener failures invisible to this starter’s shutdown path.Recommend wiring the listener into the same local group:
- errgroup.Go(func() error { + eg.Go(func() error { for { select { case <-ctx.Done(): return nil case err := <-ech: tb.Error(err) } } })This way the cleanup
eg.Wait()waits for both the runner and the listener, and no global group state is involved.Also applies to: 89-93
hack/benchmark/internal/operation/remove.go (1)
52-113: Stream receive loop can spin indefinitely on non‑EOF errorsIn
StreamRemove, the receive goroutine does:for { res, err := sc.Recv() if err == io.EOF { return nil } if err != nil { grpcError(b, err) continue } ... }For gRPC streams, any non‑
io.EOFerror fromRecvis terminal; subsequentRecvcalls will typically keep returning the same error. With the currentcontinue, this goroutine can spin forever on that error, andwg.Wait()will then block indefinitely, hanging the benchmark.Since
grpcErroris also used in non-stream paths, it’s unlikely to be callingFailNowfrom this goroutine, so this isn’t self‑terminating.Suggest exiting the goroutine on non‑EOF errors and propagating them via
errgroup:- res, err := sc.Recv() - if err == io.EOF { - return nil - } - - if err != nil { - grpcError(b, err) - continue - } + res, err := sc.Recv() + if err == io.EOF { + return nil + } + if err != nil { + grpcError(b, err) + return err + }You may also want to ensure that whatever calls these benchmarks invokes
errgroup.Wait()so the propagated error is surfaced.hack/benchmark/internal/operation/search.go (2)
56-116: StreamSearch receive loop should not continue after terminal errors
StreamSearch’s receive goroutine currently mirrors the pattern inStreamRemove:for { res, err := sc.Recv() if err == io.EOF { return nil } if err != nil { grpcError(b, err) continue } ... }Because gRPC
Recverrors (other thanio.EOF) are terminal, this will typically loop forever logging the same error and preventwg.Wait()from returning if such an error occurs.Recommend exiting the goroutine on non‑EOF errors:
- for { - res, err := sc.Recv() - if err == io.EOF { - return nil - } - if err != nil { - grpcError(b, err) - continue - } + for { + res, err := sc.Recv() + if err == io.EOF { + return nil + } + if err != nil { + grpcError(b, err) + return err + } if res.GetResponse() == nil { b.Error("returned response is nil") } }Same concern applies to the other streaming function below (
StreamSearchByID).
118-204: SearchByID looks fine; StreamSearchByID has the same Recv error‑loop risk
The non‑stream
SearchByIDchange to derivectx := b.Context()is straightforward and consistent with the rest of the API; logic otherwise unchanged.In
StreamSearchByID, the receive goroutine has:for sc != nil { res, err := sc.Recv() if err == io.EOF { finished.Store(true) return nil } if err != nil { grpcError(b, err) continue } ... }As with
StreamSearch, a non‑EOF error fromRecvis terminal. With the currentcontinue, the goroutine can spin indefinitely on the same error, never settingfinishedto true and never lettingwg.Wait()complete.Consider exiting on non‑EOF errors and propagating them via
errgroup:- for sc != nil { - res, err := sc.Recv() - if err == io.EOF { - finished.Store(true) - return nil - } - if err != nil { - grpcError(b, err) - continue - } + for sc != nil { + res, err := sc.Recv() + if err == io.EOF { + finished.Store(true) + return nil + } + if err != nil { + grpcError(b, err) + return err + } if res.GetResponse() == nil { b.Error("returned response is nil") } }You might also consider whether
sc != nilin the loop condition is still needed sincescis never mutated in this function.
🧹 Nitpick comments (2)
hack/benchmark/internal/operation/insert.go (1)
67-137: StreamInsert mixeserrgroup.Gowith a localsync.WaitGroup; consider simplifyingThe Recv side is started with
errgroup.Gobut you never callerrgroup.Wait()here, and you also wrap it in async.WaitGroupthat is what actually gates return:wg := sync.WaitGroup{} wg.Add(1) ... errgroup.Go(func() error { defer wg.Done() for { res, err := sc.Recv() if err == io.EOF { return nil } if err != nil { return err } ... atomic.AddInt64(&insertedNum, 1) } }) ... wg.Wait()As written,
errgroupdoesn’t add much over a plaingostatement, and any non-EOF error fromRecvis only surfaced through the global errgroup (if anything callserrgroup.Waitelsewhere).Two simpler options:
- If you want everything local to this benchmark: introduce a local group and drop
sync.WaitGroup:- wg := sync.WaitGroup{} - wg.Add(1) + eg, ctx := errgroup.New(ctx) b.ResetTimer() - errgroup.Go(func() error { - defer wg.Done() + eg.Go(func() error { for { res, err := sc.Recv() if err == io.EOF { return nil } if err != nil { - return err + return err // causes eg.Wait() to fail } ... } }) ... - wg.Wait() + if err := eg.Wait(); err != nil { + b.Fatal(err) + }
- Or, if you don’t need errgroup here at all, replace
errgroup.Gowith a plaingoand keep thesync.WaitGroup.Either way, you avoid relying on global
errgroupstate and make error handling for the Recv loop explicit and local to this function.hack/benchmark/internal/operation/operation.go (1)
51-67: CreateIndex behavior change (PoolSize and context) looks reasonable; confirm intended PoolSize
CreateIndexnow:
- Uses
ctx := b.Context(), which correctly ties index creation to the benchmark lifecycle.- Sets
PoolSize: 16instead of the previous larger value, which will significantly change benchmark behavior (less concurrent indexing work).If the goal of this PR is strictly to fix gRPC benchmark behavior, it may be worth double‑checking that the
PoolSizereduction is an intentional tuning change and not accidental.Otherwise, the error‑handling around
codes.FailedPreconditionand the use ofb.ResetTimer()andb.Run("CreateIndex", ...)remain consistent and look fine.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
hack/benchmark/e2e/agent/core/ngt/ngt_bench_test.go(2 hunks)hack/benchmark/internal/operation/insert.go(5 hunks)hack/benchmark/internal/operation/operation.go(2 hunks)hack/benchmark/internal/operation/remove.go(4 hunks)hack/benchmark/internal/operation/search.go(6 hunks)hack/benchmark/internal/starter/agent/core/ngt/ngt.go(3 hunks)hack/benchmark/internal/starter/gateway/vald/vald.go(1 hunks)hack/benchmark/internal/starter/starter.go(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: kpango
Repo: vdaas/vald PR: 2491
File: pkg/gateway/lb/handler/grpc/handler.go:306-316
Timestamp: 2024-06-10T19:25:49.832Z
Learning: User: kpango
PR: vdaas/vald#2491
File: pkg/gateway/lb/handler/grpc/handler.go:306-316
Timestamp: 2024-05-07T04:15:11.886Z
Learning: In this project, vtprotobuf is used, which provides enhanced methods for protobuf objects: CloneVT (object cloning), EqualVT (object comparison), SizeVT (returns object size), MarshalVT (faster version of proto.Marshal), and UnmarshalVT (faster version of proto.Unmarshal). These methods offer better performance than usual protobuf usage.
Learnt from: kpango
Repo: vdaas/vald PR: 2491
File: pkg/gateway/lb/handler/grpc/handler.go:306-316
Timestamp: 2024-10-08T20:16:00.642Z
Learning: User: kpango
PR: vdaas/vald#2491
File: pkg/gateway/lb/handler/grpc/handler.go:306-316
Timestamp: 2024-05-07T04:15:11.886Z
Learning: In this project, vtprotobuf is used, which provides enhanced methods for protobuf objects: CloneVT (object cloning), EqualVT (object comparison), SizeVT (returns object size), MarshalVT (faster version of proto.Marshal), and UnmarshalVT (faster version of proto.Unmarshal). These methods offer better performance than usual protobuf usage.
Learnt from: Matts966
Repo: vdaas/vald PR: 2928
File: pkg/gateway/lb/handler/grpc/linear_search.go:271-333
Timestamp: 2025-04-15T05:32:26.428Z
Learning: For the vald project, refactoring PRs should be kept separate from semantic changes that alter behavior. The current PR "Divide LB Gateway handler.go" is focused solely on splitting the file into smaller, more manageable components without changing functionality.
Learnt from: kpango
Repo: vdaas/vald PR: 0
File: :0-0
Timestamp: 2025-05-01T08:44:07.726Z
Learning: PR #2927 addressed gRPC connection panic issues by removing the singleflight.Group[pool.Conn] field from the gRPCClient struct in internal/net/grpc/client.go and introducing a centralized handleError function for consistent error processing in both Connect and Disconnect methods.
Learnt from: kpango
Repo: vdaas/vald PR: 0
File: :0-0
Timestamp: 2025-05-01T08:44:07.726Z
Learning: PR #2927 addressed gRPC connection panic issues by removing the singleflight.Group[pool.Conn] field from the gRPCClient struct in internal/net/grpc/client.go and introducing a centralized handleError function for more consistent error processing in both Connect and Disconnect methods.
📚 Learning: 2024-06-10T19:25:49.832Z
Learnt from: kpango
Repo: vdaas/vald PR: 2491
File: pkg/gateway/lb/handler/grpc/handler.go:306-316
Timestamp: 2024-06-10T19:25:49.832Z
Learning: User: kpango
PR: vdaas/vald#2491
File: pkg/gateway/lb/handler/grpc/handler.go:306-316
Timestamp: 2024-05-07T04:15:11.886Z
Learning: In this project, vtprotobuf is used, which provides enhanced methods for protobuf objects: CloneVT (object cloning), EqualVT (object comparison), SizeVT (returns object size), MarshalVT (faster version of proto.Marshal), and UnmarshalVT (faster version of proto.Unmarshal). These methods offer better performance than usual protobuf usage.
Applied to files:
hack/benchmark/internal/starter/gateway/vald/vald.gohack/benchmark/internal/starter/agent/core/ngt/ngt.gohack/benchmark/internal/operation/operation.go
📚 Learning: 2025-04-15T05:32:26.428Z
Learnt from: Matts966
Repo: vdaas/vald PR: 2928
File: pkg/gateway/lb/handler/grpc/linear_search.go:271-333
Timestamp: 2025-04-15T05:32:26.428Z
Learning: For the vald project, refactoring PRs should be kept separate from semantic changes that alter behavior. The current PR "Divide LB Gateway handler.go" is focused solely on splitting the file into smaller, more manageable components without changing functionality.
Applied to files:
hack/benchmark/internal/starter/gateway/vald/vald.go
📚 Learning: 2025-06-04T06:36:35.397Z
Learnt from: Matts966
Repo: vdaas/vald PR: 3000
File: tests/v2/e2e/crud/kubernetes_test.go:138-147
Timestamp: 2025-06-04T06:36:35.397Z
Learning: In E2E tests for the Vald project, Matts966 prefers using dual error logging (both fmt.Fprintf to stderr and t.Errorf) when handling Kubernetes operations to provide immediate debugging feedback during test execution alongside structured test failure reporting.
Applied to files:
hack/benchmark/internal/starter/gateway/vald/vald.gohack/benchmark/internal/operation/operation.go
📚 Learning: 2025-05-01T08:44:07.726Z
Learnt from: kpango
Repo: vdaas/vald PR: 0
File: :0-0
Timestamp: 2025-05-01T08:44:07.726Z
Learning: PR #2927 removed the singleflight.Group field from the gRPCClient struct in internal/net/grpc/client.go and introduced a centralized handleError function for error processing to address gRPC connection panic issues.
Applied to files:
hack/benchmark/internal/starter/agent/core/ngt/ngt.go
🧬 Code graph analysis (4)
hack/benchmark/internal/operation/insert.go (2)
hack/benchmark/internal/assets/dataset.go (1)
Dataset(30-43)internal/io/io.go (1)
EOF(40-40)
hack/benchmark/internal/starter/agent/core/ngt/ngt.go (1)
internal/sync/errgroup/group.go (1)
Wait(247-249)
hack/benchmark/internal/operation/remove.go (2)
internal/runner/runner.go (1)
Run(141-242)internal/io/io.go (1)
EOF(40-40)
hack/benchmark/internal/operation/search.go (2)
hack/benchmark/internal/assets/dataset.go (1)
Dataset(30-43)internal/sync/errgroup/group.go (1)
Wait(247-249)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (8)
hack/benchmark/internal/starter/agent/core/ngt/ngt.go (1)
49-63: Run(tb testing.TB) refactor looks consistent with new starter APIDeriving
ctxfromtb.Context()and using a localerrgroup.Grouparoundrunner.Runplus a cancelable context in the returned cleanup func matches the newStarter.Run(tb testing.TB) func()contract and should scope the benchmark lifecycle cleanly.Also applies to: 89-93
hack/benchmark/internal/operation/insert.go (1)
30-65: Insert benchmark change aligns with new no-ctx APISwitching to
Insert(b *testing.B, ds assets.Dataset)and derivingctx := b.Context()internally keeps callers simple while still propagating benchmark cancellation into the client. The insert loop and AlreadyExists handling remain consistent with the previous behavior.hack/benchmark/internal/starter/gateway/vald/vald.go (1)
37-43: Gateway starter Run signature matches updated Starter interfaceAdapting
(*server).RuntoRun(tb testing.TB) func()keeps this gateway starter consistent with the newstarter.StarterAPI. Given it’s currently a stub, this change is safe and future-proof.hack/benchmark/internal/starter/starter.go (1)
24-26: Starter interface update is coherent with callers and implementersChanging
Starter.RuntoRun(testing.TB) func()matches the updated agent and gateway starters and the benchmarks that now callRun(b)(). This keeps the starter abstraction consistent and removes the need for an external context parameter.hack/benchmark/e2e/agent/core/ngt/ngt_bench_test.go (1)
67-73: Benchmark updates correctly follow new Run and operation APIsThe benchmarks now:
- Start the NGT agent with
defer ngt.New(...).Run(b)(); and- Call the no-context operation methods (
Insert,CreateIndex,Search*,Remove*) that derivectxfromb.Context()internally.This wiring is consistent with the starter and operation refactors and keeps the benchmark code straightforward.
Also applies to: 79-83, 107-113, 119-123
hack/benchmark/internal/operation/remove.go (1)
26-49: Context source change inRemovelooks consistentUsing
ctx := b.Context()and dropping the explicitcontext.Contextparameter keeps the benchmark tied to thetesting.Blifecycle and matches the interface changes inoperation.go. The rest of the logic is unchanged and looks fine.hack/benchmark/internal/operation/search.go (1)
28-54: Non‑streamSearchcorrectly switched tob.Context()Dropping the explicit
context.Contextparameter and usingctx := b.Context()inside the benchmark aligns with the updatedOperationinterface and keeps RPCs bound to the benchmark’s lifecycle. The rest of the logic is unchanged and looks correct.hack/benchmark/internal/operation/operation.go (1)
26-36: Operation interface change is correctly applied throughout; verification confirms no legacy signatures remainAll implementations of the
Operationinterface inhack/benchmark/internal/operation/search.gohave been properly updated to match the new signatures (e.g.,Search(b *testing.B, ds assets.Dataset)withoutcontext.Context). The implementations correctly derive context viab.Context()internally when needed. Commented-out test code insearch_test.goreferences the old signature but is not active. No mismatches or remaining old signatures found in the codebase.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3342 +/- ##
==========================================
- Coverage 18.12% 18.11% -0.01%
==========================================
Files 126 126
Lines 12038 12042 +4
==========================================
Hits 2182 2182
- Misses 9583 9587 +4
Partials 273 273 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Description
Related Issue
Versions
Checklist
Special notes for your reviewer
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.