refactor: Use generics in function of bidirectional stream #2803
Conversation
📝 WalkthroughWalkthroughThe pull request introduces type-related modifications across multiple files in the gRPC and load testing components. The changes primarily focus on updating function signatures to use more explicit type parameters and return pointers to Changes
Sequence DiagramsequenceDiagram
participant Client
participant DataProvider
participant Stream
Client->>DataProvider: Request data
DataProvider-->>Client: Return *any data
Client->>Stream: Send data pointer
Stream->>Stream: Process data
Stream-->>Client: Return result/error
The sequence diagram illustrates the updated flow where data providers now return pointers, which are then passed through the stream processing mechanism, maintaining the core logic while introducing more explicit type handling. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
pkg/tools/cli/loadtest/service/insert.go (1)
Line range hint
12-24: Consider revising the discount and fee structure.The current implementation has potential issues:
- For small purchase amounts, the $20 flat fee on discounted orders can exceed the discount benefit.
- A customer with 3-4 years loyalty (10% discount) buying a $100 item would pay $110 ($100 - $10 + $20), which is more than without any discount ($100).
Consider one of these alternatives:
- Apply the fee as a percentage instead of a flat amount
- Only apply the fee when the discount benefit exceeds the fee
- Scale the fee based on the purchase amount
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
internal/net/grpc/stream.go(4 hunks)pkg/tools/cli/loadtest/service/insert.go(2 hunks)pkg/tools/cli/loadtest/service/loader.go(7 hunks)pkg/tools/cli/loadtest/service/search.go(1 hunks)
🔇 Additional comments (9)
pkg/tools/cli/loadtest/service/insert.go (1)
Line range hint
4-4: Add unit tests as indicated by the TODO comment.The TODO comment indicates missing tests for this functionality.
Would you like me to help generate comprehensive unit tests for these functions?
internal/net/grpc/stream.go (1)
170-171: LGTM! Good use of generics for type safety.The updated function signature using generics (
BidirectionalStreamClient[S, R any]) and pointer types improves type safety and makes the API more explicit.pkg/tools/cli/loadtest/service/loader.go (6)
52-52: LGTM! Improved naming and type safety.The rename from
dataProvidertosendDataProviderbetter reflects its usage in bidirectional streams, and returning a pointer toanyis more memory efficient.
99-101: LGTM! Consistent provider assignment.The assignments correctly use the renamed
sendDataProviderfield while maintaining proper error handling.
Line range hint
137-142: LGTM! Consistent pointer usage in callback.The callback signature now correctly accepts a pointer to
any, maintaining consistency with the broader refactor while preserving the atomic counter functionality.
186-186: LGTM! Consistent callback signature.The method signature correctly uses a pointer type for the callback parameter, maintaining consistency with the broader refactor.
204-204: LGTM! Updated stream client usage.The BidirectionalStreamClient call correctly uses the renamed sendDataProvider.
Line range hint
213-225: Verify error handling with pointer usage.While the changes to use pointers are correct, ensure that proper nil checks are in place throughout the error handling path.
✅ Verification successful
Error handling with pointer usage is properly implemented
The error handling chain is complete and pointer usage is safe throughout the error path. The results are properly scoped and initialized before being passed by address to the callback function.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for error handling patterns with pointer usage rg -U "if\s+.*err\s*!=\s*nil.*\{[\s\S]*?f\(&.*,\s*err\)" .Length of output: 11178
pkg/tools/cli/loadtest/service/search.go (1)
35-39: Verify type assertion safety.The type assertion
v.([]float32)could panic if the dataset returns an unexpected type.
internal/net/grpc/stream.go
Outdated
| func BidirectionalStreamClient[S, R any]( | ||
| stream ClientStream, sendDataProvider func() *S, callBack func(*R, error), |
There was a problem hiding this comment.
@highpon Thank you for your contribution around important libs.
How about an interface that cancels all stream processing in case of false by using a boolean as the return value of the callback function argument of the BidirectionalStreamClient?
It would be very useful to have an interface that can stop all streams when a fatal error is detected in the callback function.
| func BidirectionalStreamClient[S, R any]( | |
| stream ClientStream, sendDataProvider func() *S, callBack func(*R, error), | |
| func BidirectionalStreamClient[S, R any]( | |
| stream ClientStream, sendDataProvider func() *S, callBack func(*R, error) bool, |
There was a problem hiding this comment.
Thanks for your review!
fix: 39c2149
internal/net/grpc/stream.go
Outdated
| err = stream.RecvMsg(res) | ||
| if err == io.EOF || errors.Is(err, io.EOF) { | ||
| cancel() | ||
| return nil | ||
| } | ||
| f(res, err) | ||
| callBack(res, err) |
There was a problem hiding this comment.
Kind of like this.
| callBack(res, err) | |
| if !callBack(res, err) { | |
| cancel() | |
| return nil | |
| } |
There was a problem hiding this comment.
Thanks for you review!
fix: 39c2149
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
pkg/tools/cli/loadtest/service/loader.go (4)
52-52: LGTM! Consider adding documentation.The change to use
*anyand the renamed field better reflects its purpose. Consider adding a doc comment to explain the purpose of this field and why it returns a pointer.+// sendDataProvider returns the next data item to be sent in the load test. +// It returns a pointer to optimize for large data structures and returns nil when no more data is available. sendDataProvider func() *any
137-137: Consider whether pointer type is necessary for the callback.Since this callback is only used for progress tracking and error counting, receiving a pointer to
anymight be unnecessary overhead. Consider keeping it asfunc(i any, err error)unless there's a specific need for pointer semantics in the callback.
Line range hint
213-225: Simplify pointer handling in unary requests.The current implementation unnecessarily dereferences and re-addresses the pointer. Consider simplifying:
-r := l.sendDataProvider() -if r == nil { - break -} -// ... -res, err := l.loaderFunc(egctx, conn, *r) -f(&res, err) +req := l.sendDataProvider() +if req == nil { + break +} +// ... +res, err := l.loaderFunc(egctx, conn, *req) +f(req, err)This change:
- Uses clearer variable naming
- Avoids unnecessary pointer operations
- Reuses the existing pointer in the callback
Line range hint
52-225: Consider documenting the pointer-based design decision.The transition to pointer-based types is a significant architectural change that could affect performance and memory usage. Consider:
- Adding a comment in the package documentation explaining the rationale behind using pointers to
any- Documenting any performance improvements or memory savings observed
- Adding benchmarks to validate the benefits of this approach
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/tools/cli/loadtest/service/loader.go(7 hunks)
🔇 Additional comments (3)
pkg/tools/cli/loadtest/service/loader.go (3)
99-101: LGTM! Changes align with the new pointer-based approach.The assignments correctly use the renamed field and maintain the existing logic while adapting to the new pointer type.
186-186: Consistent with previous callback changes.
204-204: LGTM! Appropriate use of pointer-based provider in streaming context.The change correctly adapts the bidirectional streaming to use the pointer-based provider, which is beneficial for performance in streaming scenarios.
There was a problem hiding this comment.
Actionable comments posted: 13
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/net/grpc/stream.go(5 hunks)pkg/tools/cli/loadtest/service/insert.go(2 hunks)pkg/tools/cli/loadtest/service/loader.go(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/net/grpc/stream.go
🔇 Additional comments (2)
pkg/tools/cli/loadtest/service/loader.go (2)
100-100: 🛠️ Refactor suggestionAdjust data provider assignment to match function signature
After changing the return type of
insertRequestProvider, ensure the assignment matches.Apply this diff:
- l.sendDataProvider, l.dataSize, err = insertRequestProvider(dataset, l.batchSize) + l.sendDataProvider, l.dataSize, err = insertRequestProvider(dataset, l.batchSize)Likely invalid or redundant comment.
208-209: 🛠️ Refactor suggestionAdjust callback function to match updated parameter types
After changing
fto acceptany, ensure the callback passes the correct type.Apply this diff:
- f(nil, err) + f(nil, err)Likely invalid or redundant comment.
vankichi
requested review from
a team and
kmrmt
and removed request for
a team
Description
Use generics of function of bidirectional stream.
This PR intends to refactor current code.
Related Issue
Versions
Checklist
Special notes for your reviewer
Summary by CodeRabbit
Refactor
Technical Improvements