Fix gRPC error msg handling for lb-gateway handler #2663
Conversation
Deploying vald with
|
| Latest commit: |
59a39bc
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://a643d56d.vald.pages.dev |
| Branch Preview URL: | https://refactor-pkg-fix-handler-grp.vald.pages.dev |
📝 Walkthrough📝 WalkthroughWalkthroughThe changes involve modifications to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 (
|
|
[CHATOPS:HELP] ChatOps commands.
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2663 +/- ##
==========================================
+ Coverage 23.99% 24.06% +0.07%
==========================================
Files 539 539
Lines 47179 47070 -109
==========================================
+ Hits 11319 11329 +10
+ Misses 35090 34972 -118
+ Partials 770 769 -1 ☔ View full report in Codecov by Sentry. |
vankichi
force-pushed
the
refactor/pkg/fix-handler-gRPC-errro-handling
branch
from
3933a21
to
efbfe88
Compare
vankichi
force-pushed
the
refactor/pkg/fix-handler-gRPC-errro-handling
branch
from
efbfe88
to
0303819
Compare
Signed-off-by: vankichi <[email protected]>
vankichi
force-pushed
the
refactor/pkg/fix-handler-gRPC-errro-handling
branch
from
0303819
to
59a39bc
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (11)
pkg/gateway/lb/handler/grpc/handler.go (10)
258-270: LGTM! Consider adding unit tests for this section.The error handling and status code assignment logic in this section are well-implemented. The use of
status.FromErroris a good practice for handling gRPC errors. However, this newly added code is not covered by tests.To improve the overall quality and maintainability of the codebase, consider adding unit tests to cover this section of the
Existsmethod, particularly focusing on different error scenarios and status code assignments.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 258-260: pkg/gateway/lb/handler/grpc/handler.go#L258-L260
Added lines #L258 - L260 were not covered by tests
[warning] 262-268: pkg/gateway/lb/handler/grpc/handler.go#L262-L268
Added lines #L262 - L268 were not covered by tests
[warning] 270-270: pkg/gateway/lb/handler/grpc/handler.go#L270
Added line #L270 was not covered by tests
313-313: LGTM! Consider adding unit tests for this section.The use of
doSearchwith a function argument is a good practice for code reuse and flexibility. However, this newly added code is not covered by tests.To ensure the reliability of the
Searchmethod, consider adding unit tests that cover this call todoSearch, including various scenarios for the function argument passed to it.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 313-313: pkg/gateway/lb/handler/grpc/handler.go#L313
Added line #L313 was not covered by tests
318-320: LGTM! Consider adding unit tests for error handling.The error handling and span recording in this section are appropriate for maintaining observability. However, this newly added code is not covered by tests.
To ensure robust error handling, consider adding unit tests that cover these error scenarios in the
Searchmethod. This will help verify that span attributes and statuses are correctly set when errors occur.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 318-318: pkg/gateway/lb/handler/grpc/handler.go#L318
Added line #L318 was not covered by tests
[warning] 320-320: pkg/gateway/lb/handler/grpc/handler.go#L320
Added line #L320 was not covered by tests
370-377: LGTM! Consider adding unit tests for fallback mechanism.The error handling and fallback mechanism to the agent's SearchByID method are well-implemented. The use of
status.FromErroris consistent with other parts of the code. However, this newly added code is not covered by tests.To ensure the reliability of the
SearchByIDmethod, consider adding unit tests that cover this error handling and fallback mechanism. Specifically, test scenarios where the initial search fails and the fallback to the agent's SearchByID is triggered.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 370-370: pkg/gateway/lb/handler/grpc/handler.go#L370
Added line #L370 was not covered by tests
[warning] 377-377: pkg/gateway/lb/handler/grpc/handler.go#L377
Added line #L377 was not covered by tests
490-493: LGTM! Consider adding unit tests for error handling in StreamSearch.The error handling and span recording in the
StreamSearchmethod are appropriate for maintaining observability. The use ofstatus.FromErroris consistent with other parts of the code. However, this newly added code is not covered by tests.To ensure robust error handling in the streaming context, consider adding unit tests that cover these error scenarios in the
StreamSearchmethod. This will help verify that span attributes and statuses are correctly set when errors occur during streaming operations.
509-512: LGTM! Consider adding unit tests for final error handling in StreamSearch.The final error handling and span recording in the
StreamSearchmethod are appropriate for maintaining observability. The use ofstatus.FromErroris consistent with other parts of the code. However, this newly added code is not covered by tests.To ensure comprehensive error handling, consider adding unit tests that cover this final error check in the
StreamSearchmethod. This will help verify that span attributes and statuses are correctly set when errors occur at the end of the streaming operation.
537-540: LGTM! Consider adding unit tests for error handling in StreamSearchByID.The error handling and span recording in the
StreamSearchByIDmethod are appropriate for maintaining observability. The use ofstatus.FromErroris consistent with other parts of the code. However, this newly added code is not covered by tests.To ensure robust error handling in the streaming context, consider adding unit tests that cover these error scenarios in the
StreamSearchByIDmethod. This will help verify that span attributes and statuses are correctly set when errors occur during streaming operations.
556-559: LGTM! Consider adding unit tests for final error handling in StreamSearchByID.The final error handling and span recording in the
StreamSearchByIDmethod are appropriate for maintaining observability. The use ofstatus.FromErroris consistent with other parts of the code. However, this newly added code is not covered by tests.To ensure comprehensive error handling, consider adding unit tests that cover this final error check in the
StreamSearchByIDmethod. This will help verify that span attributes and statuses are correctly set when errors occur at the end of the streaming operation.
624-628: LGTM! Consider adding unit tests for error handling in MultiSearch.The error handling and span recording in the
MultiSearchmethod are appropriate for maintaining observability. The use ofstatus.FromErroris consistent with other parts of the code. However, this newly added code is not covered by tests.To ensure robust error handling for multiple search operations, consider adding unit tests that cover these error scenarios in the
MultiSearchmethod. This will help verify that span attributes and statuses are correctly set when errors occur during multiple search operations.
693-697: LGTM! Consider adding unit tests for error handling in MultiSearchByID.The error handling and span recording in the
MultiSearchByIDmethod are appropriate for maintaining observability. The use ofstatus.FromErroris consistent with other parts of the code. However, this newly added code is not covered by tests.To ensure robust error handling for multiple search-by-ID operations, consider adding unit tests that cover these error scenarios in the
MultiSearchByIDmethod. This will help verify that span attributes and statuses are correctly set when errors occur during multiple search-by-ID operations.pkg/gateway/lb/handler/grpc/aggregation.go (1)
34-34: Consider aliasing the importedsyncpackage to avoid confusionImporting the internal
syncpackage assyncmay cause confusion with Go's standard librarysyncpackage. To improve code clarity and prevent potential conflicts, consider aliasing the import. For example:import ( ... - "github.com/vdaas/vald/internal/sync" + internalSync "github.com/vdaas/vald/internal/sync" )This helps differentiate between the standard
syncpackage and the internal one.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- pkg/gateway/lb/handler/grpc/aggregation.go (8 hunks)
- pkg/gateway/lb/handler/grpc/handler.go (57 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
pkg/gateway/lb/handler/grpc/aggregation.go
[warning] 60-60: pkg/gateway/lb/handler/grpc/aggregation.go#L60
Added line #L60 was not covered by tests
[warning] 239-239: pkg/gateway/lb/handler/grpc/aggregation.go#L239
Added line #L239 was not covered by tests
[warning] 242-242: pkg/gateway/lb/handler/grpc/aggregation.go#L242
Added line #L242 was not covered by tests
[warning] 245-245: pkg/gateway/lb/handler/grpc/aggregation.go#L245
Added line #L245 was not covered by tests
[warning] 266-266: pkg/gateway/lb/handler/grpc/aggregation.go#L266
Added line #L266 was not covered by tests
[warning] 269-269: pkg/gateway/lb/handler/grpc/aggregation.go#L269
Added line #L269 was not covered by tests
[warning] 272-272: pkg/gateway/lb/handler/grpc/aggregation.go#L272
Added line #L272 was not covered by tests
[warning] 287-287: pkg/gateway/lb/handler/grpc/aggregation.go#L287
Added line #L287 was not covered by tests
[warning] 290-290: pkg/gateway/lb/handler/grpc/aggregation.go#L290
Added line #L290 was not covered by tests
[warning] 293-293: pkg/gateway/lb/handler/grpc/aggregation.go#L293
Added line #L293 was not covered by tests
[warning] 308-308: pkg/gateway/lb/handler/grpc/aggregation.go#L308
Added line #L308 was not covered by tests
[warning] 311-311: pkg/gateway/lb/handler/grpc/aggregation.go#L311
Added line #L311 was not covered by tests
[warning] 316-316: pkg/gateway/lb/handler/grpc/aggregation.go#L316
Added line #L316 was not covered by tests
[warning] 332-332: pkg/gateway/lb/handler/grpc/aggregation.go#L332
Added line #L332 was not covered by tests
[warning] 335-335: pkg/gateway/lb/handler/grpc/aggregation.go#L335
Added line #L335 was not covered by tests
[warning] 338-338: pkg/gateway/lb/handler/grpc/aggregation.go#L338
Added line #L338 was not covered by tests
[warning] 345-345: pkg/gateway/lb/handler/grpc/aggregation.go#L345
Added line #L345 was not covered by tests
[warning] 348-348: pkg/gateway/lb/handler/grpc/aggregation.go#L348
Added line #L348 was not covered by tests
[warning] 363-363: pkg/gateway/lb/handler/grpc/aggregation.go#L363
Added line #L363 was not covered by tests
[warning] 366-366: pkg/gateway/lb/handler/grpc/aggregation.go#L366
Added line #L366 was not covered by tests
[warning] 369-369: pkg/gateway/lb/handler/grpc/aggregation.go#L369
Added line #L369 was not covered by tests
[warning] 372-372: pkg/gateway/lb/handler/grpc/aggregation.go#L372
Added line #L372 was not covered by testspkg/gateway/lb/handler/grpc/handler.go
[warning] 258-260: pkg/gateway/lb/handler/grpc/handler.go#L258-L260
Added lines #L258 - L260 were not covered by tests
[warning] 262-268: pkg/gateway/lb/handler/grpc/handler.go#L262-L268
Added lines #L262 - L268 were not covered by tests
[warning] 270-270: pkg/gateway/lb/handler/grpc/handler.go#L270
Added line #L270 was not covered by tests
[warning] 313-313: pkg/gateway/lb/handler/grpc/handler.go#L313
Added line #L313 was not covered by tests
[warning] 318-318: pkg/gateway/lb/handler/grpc/handler.go#L318
Added line #L318 was not covered by tests
[warning] 320-320: pkg/gateway/lb/handler/grpc/handler.go#L320
Added line #L320 was not covered by tests
[warning] 370-370: pkg/gateway/lb/handler/grpc/handler.go#L370
Added line #L370 was not covered by tests
[warning] 377-377: pkg/gateway/lb/handler/grpc/handler.go#L377
Added line #L377 was not covered by tests
|
[FOSSA] The scan result will be available at https://app.fossa.com/projects/custom%2B21465%2Fvald/refs/branch/refactor%2Fpkg%2Ffix-handler-gRPC-errro-handling/389a22bad3bf38688eb0bf490534aa3d29466405 |
Profile Report
|
vankichi
requested review from
a team and
kmrmt
and removed request for
a team
Description
SSIA
Related Issue
Versions
Checklist
Special notes for your reviewer
Summary by CodeRabbit
New Features
Bug Fixes
Refactor