add Search_Config validation #2811
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
📝 WalkthroughWalkthroughThis pull request introduces a new error variable Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
⏰ Context from checks skipped due to timeout of 90000ms (100)
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 (
|
kmrmt
force-pushed
the
bugfix/grpc/panic-caused-with-nil-search-config
branch
from
eb8e500 to
434d801
Compare
Deploying vald with
|
| Latest commit: |
063b857
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://3f043c5a.vald.pages.dev |
| Branch Preview URL: | https://bugfix-grpc-panic-caused-wit.vald.pages.dev |
|
[CHATOPS:HELP] ChatOps commands.
|
github-actions
bot
added
area/internal
area/gateway/lb
size/S
type/bug
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2811 +/- ##
==========================================
- Coverage 23.92% 23.91% -0.02%
==========================================
Files 546 547 +1
Lines 54555 54586 +31
==========================================
- Hits 13054 13053 -1
- Misses 40717 40747 +30
- Partials 784 786 +2 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
kmrmt
requested review from
a team,
kpango and
vankichi
and removed request for
a team
Profile Report
|
vankichi
reviewed
Jan 27, 2025
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/errors/lb.go(1 hunks)pkg/gateway/lb/handler/grpc/aggregation.go(1 hunks)pkg/gateway/lb/handler/grpc/handler.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/errors/lb.go
- pkg/gateway/lb/handler/grpc/aggregation.go
🔇 Additional comments (2)
pkg/gateway/lb/handler/grpc/handler.go (2)
441-461: LGTM! The Search_Config validation is well implemented.The validation logic for nil Search_Config is properly implemented with:
- Clear error message and wrapping
- Detailed error details including request info and field violations
- Proper error tracing and status code setting
441-461: Verify error handling in caller methods.Since doSearch is called by multiple methods (Search, SearchByID, LinearSearch), verify that they properly handle the new ErrInvalidSearchConfig error. Let's check the error handling:
✅ Verification successful
Error handling is properly implemented in all caller methods
The new ErrInvalidSearchConfig error from doSearch is properly handled by all caller methods (Search, LinearSearch) with appropriate error propagation, span recording, and status code setting. The implementation is consistent with the codebase's error handling patterns.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Find all methods that call doSearch and verify their error handling ast-grep --pattern 'res, attrs, err := s.doSearch($$$)'Length of output: 905
Script:
#!/bin/bash # Get the full implementation of Search and LinearSearch methods ast-grep --pattern 'func (s *handler) Search(ctx context.Context, $$$) (*payload.Search_Response, error) { $$$ }' echo "---" ast-grep --pattern 'func (s *handler) LinearSearch(ctx context.Context, $$$) (*payload.Search_Response, error) { $$$ }'Length of output: 256
Script:
#!/bin/bash # Search for the method definitions with context rg -U "func \(s \*handler\) (Search|LinearSearch)" pkg/gateway/lb/handler/grpc/handler.go -B 2 -A 30Length of output: 102
Script:
#!/bin/bash # First verify the file exists ls -l pkg/gateway/lb/handler/grpc/handler.go echo "=== Searching for method names and error handling ===" # Use basic grep patterns grep -A 5 -B 5 "Search\|LinearSearch\|err :=" pkg/gateway/lb/handler/grpc/handler.goLength of output: 34188
kmrmt
force-pushed
the
bugfix/grpc/panic-caused-with-nil-search-config
branch
from
d3b4f18 to
8976c4a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/gateway/lb/handler/grpc/handler.go (1)
441-461: Consider improving error message clarity.The error message "search config is nil in doSearch" could be more descriptive. Consider enhancing it to better explain the impact and required action.
Example:
- err = errors.ErrInvalidSearchConfig("search config is nil in doSearch") + err = errors.ErrInvalidSearchConfig("Search_Config is required for performing search operations - please provide valid search configuration")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/errors/lb.go(1 hunks)pkg/gateway/lb/handler/grpc/aggregation.go(1 hunks)pkg/gateway/lb/handler/grpc/handler.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/errors/lb.go
- pkg/gateway/lb/handler/grpc/aggregation.go
⏰ Context from checks skipped due to timeout of 90000ms (67)
- GitHub Check: Run tests for internal packages
- GitHub Check: Run tests for pkg packages
- GitHub Check: CodeQL
- GitHub Check: check-format-diff
- GitHub Check: runner / go build
- GitHub Check: coverage
- GitHub Check: build / build
- GitHub Check: Run tests for internal packages
- GitHub Check: Run tests for pkg packages
- GitHub Check: CodeQL
- GitHub Check: check-format-diff
- GitHub Check: runner / go build
- GitHub Check: coverage
- GitHub Check: build / build
- GitHub Check: Run tests for internal packages
- GitHub Check: Run tests for pkg packages
- GitHub Check: CodeQL
- GitHub Check: check-format-diff
- GitHub Check: runner / go build
- GitHub Check: coverage
- GitHub Check: build / build
- GitHub Check: Run tests for internal packages
- GitHub Check: Run tests for pkg packages
- GitHub Check: CodeQL
- GitHub Check: check-format-diff
- GitHub Check: runner / go build
- GitHub Check: coverage
- GitHub Check: build / build
- GitHub Check: Run tests for internal packages
- GitHub Check: Run tests for pkg packages
- GitHub Check: CodeQL
- GitHub Check: check-format-diff
- GitHub Check: runner / go build
- GitHub Check: coverage
- GitHub Check: build / build
- GitHub Check: Run tests for internal packages
- GitHub Check: Run tests for pkg packages
- GitHub Check: CodeQL
- GitHub Check: check-format-diff
- GitHub Check: runner / go build
- GitHub Check: coverage
- GitHub Check: build / build
- GitHub Check: Run tests for internal packages
- GitHub Check: Run tests for pkg packages
- GitHub Check: CodeQL
- GitHub Check: check-format-diff
- GitHub Check: runner / go build
- GitHub Check: coverage
- GitHub Check: build / build
- GitHub Check: Run tests for pkg packages
- GitHub Check: CodeQL
- GitHub Check: check-format-diff
- GitHub Check: runner / go build
- GitHub Check: coverage
- GitHub Check: build / build
- GitHub Check: Run tests for pkg packages
- GitHub Check: CodeQL
- GitHub Check: check-format-diff
- GitHub Check: runner / go build
- GitHub Check: coverage
- GitHub Check: build / build
- GitHub Check: Run tests for pkg packages
- GitHub Check: CodeQL
- GitHub Check: check-format-diff
- GitHub Check: runner / go build
- GitHub Check: coverage
- GitHub Check: build / build
🔇 Additional comments (2)
pkg/gateway/lb/handler/grpc/handler.go (2)
441-461: LGTM! Well-implemented nil config validation.The nil check for Search_Config is properly implemented with appropriate error wrapping, status codes, and tracing integration.
441-461: Add test cases for nil Search_Config validation.Please add test cases to verify:
- Error returned when Search_Config is nil
- Error details contain correct field violations
- Error is properly propagated through calling methods
Example test structure:
func TestDoSearch_NilConfig(t *testing.T) { tests := []struct { name string cfg *payload.Search_Config wantErr bool errCode codes.Code }{ { name: "nil config returns error", cfg: nil, wantErr: true, errCode: codes.InvalidArgument, }, // Add more test cases } // Implement test logic }
vankichi
previously approved these changes
Jan 28, 2025
kmrmt
dismissed stale reviews from vankichi and coderabbitai[bot]
via
9a3cb9b
kpango
approved these changes
Jan 29, 2025
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/tools/cli/loadtest/service/search.go (1)
37-41: LGTM! Good addition of default Search_Config.The initialization of Search_Config with default values aligns well with the PR's objective of preventing nil config errors. The chosen default values (Num=10, Radius=-1, Epsilon=0.1) appear reasonable for load testing purposes.
However, consider documenting why these specific default values were chosen, either in code comments or in the PR description, to help future maintainers understand the reasoning.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/tools/cli/loadtest/service/search.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (100)
- GitHub Check: Run tests for cmd packages
- GitHub Check: Run tests for pkg packages
- GitHub Check: Run tests for internal packages
- GitHub Check: runner / go build
- GitHub Check: coverage
- GitHub Check: check-format-diff
- GitHub Check: CodeQL
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: Run tests for cmd packages
- GitHub Check: Run tests for pkg packages
- GitHub Check: Run tests for internal packages
- GitHub Check: runner / go build
- GitHub Check: coverage
- GitHub Check: check-format-diff
- GitHub Check: CodeQL
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: Run tests for cmd packages
- GitHub Check: Run tests for pkg packages
- GitHub Check: Run tests for internal packages
- GitHub Check: runner / go build
- GitHub Check: coverage
- GitHub Check: check-format-diff
- GitHub Check: CodeQL
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: Run tests for cmd packages
- GitHub Check: Run tests for pkg packages
- GitHub Check: Run tests for internal packages
- GitHub Check: runner / go build
- GitHub Check: coverage
- GitHub Check: check-format-diff
- GitHub Check: CodeQL
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: Run tests for cmd packages
- GitHub Check: Run tests for pkg packages
- GitHub Check: Run tests for internal packages
- GitHub Check: runner / go build
- GitHub Check: coverage
- GitHub Check: check-format-diff
- GitHub Check: CodeQL
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: Run tests for cmd packages
- GitHub Check: Run tests for pkg packages
- GitHub Check: Run tests for internal packages
- GitHub Check: runner / go build
- GitHub Check: coverage
- GitHub Check: check-format-diff
- GitHub Check: CodeQL
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: Run tests for cmd packages
- GitHub Check: Run tests for pkg packages
- GitHub Check: Run tests for internal packages
- GitHub Check: runner / go build
- GitHub Check: coverage
- GitHub Check: check-format-diff
- GitHub Check: CodeQL
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: Run tests for cmd packages
- GitHub Check: Run tests for pkg packages
- GitHub Check: Run tests for internal packages
- GitHub Check: runner / go build
- GitHub Check: coverage
- GitHub Check: check-format-diff
- GitHub Check: CodeQL
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: Run tests for cmd packages
- GitHub Check: Run tests for pkg packages
- GitHub Check: Run tests for internal packages
- GitHub Check: runner / go build
- GitHub Check: coverage
- GitHub Check: check-format-diff
- GitHub Check: CodeQL
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: Run tests for cmd packages
- GitHub Check: Run tests for pkg packages
- GitHub Check: Run tests for internal packages
- GitHub Check: runner / go build
- GitHub Check: coverage
- GitHub Check: check-format-diff
- GitHub Check: CodeQL
- GitHub Check: build / build
- GitHub Check: build / build
- GitHub Check: build / build
🔇 Additional comments (1)
pkg/tools/cli/loadtest/service/search.go (1)
37-41: Verify test coverage with these default values.While the default configuration looks good, we should ensure these values provide adequate test coverage:
- Does Num=10 sufficiently test pagination?
- Does Radius=-1 adequately test distance constraints?
- Is Epsilon=0.1 a good precision value for testing?
Run this script to analyze test coverage and usage patterns:
✅ Verification successful
Default search configuration values have comprehensive test coverage
The test suite in
pkg/agent/core/ngt/handler/grpc/search_test.gothoroughly verifies these default values:
- Tests Num=10 against varying dataset sizes (5, 10, 20 vectors)
- Validates Radius=-1 for unrestricted search behavior
- Verifies Epsilon=0.1 precision across different vector types
- Covers both exact and approximate search scenarios
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check how these config values are used in tests and actual code # Find tests using Search_Config echo "Tests using Search_Config:" rg -A 5 "Search_Config" --glob "*_test.go" # Find actual usage patterns echo "Usage patterns in non-test code:" rg -A 5 "Search_Config" --glob "!*_test.go"Length of output: 63609
vankichi
approved these changes
Jan 29, 2025
kmrmt
force-pushed
the
bugfix/grpc/panic-caused-with-nil-search-config
branch
from
9a3cb9b to
063b857
Compare
vdaas-ci
pushed a commit
that referenced
this pull request
Jan 29, 2025
* add Search_Config validation * change to error function and generate individual errors * add search configuration
2 tasks
kmrmt
added a commit
that referenced
this pull request
Jan 30, 2025
* add Search_Config validation * change to error function and generate individual errors * add search configuration
vankichi
added a commit
that referenced
this pull request
Jan 31, 2025
* add Search_Config validation * change to error function and generate individual errors * add search configuration Co-authored-by: Kosuke Morimoto <[email protected]> Co-authored-by: Kiichiro YUKAWA <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Search_Config is required field for Search request.
However, it might be forget to set a Search_Config value, unintentionally.
Add Search_Config validation gateway and return error response from it.
Related Issue
Versions
Checklist
Special notes for your reviewer
Summary by CodeRabbit
New Features
ErrInvalidSearchConfigto handle invalid search configuration scenarios.Bug Fixes
Improvements