Backport PR #2584 to release/v1.7 for Implement ngt property get API #2588
Conversation
* add property to proto Signed-off-by: Kosuke Morimoto <[email protected]> * build proto files Signed-off-by: Kosuke Morimoto <[email protected]> * fix Signed-off-by: Kosuke Morimoto <[email protected]> * fix rust API Signed-off-by: Kosuke Morimoto <[email protected]> * fix unit test Signed-off-by: Kosuke Morimoto <[email protected]> * make format Signed-off-by: Kosuke Morimoto <[email protected]> * make format Signed-off-by: Kosuke Morimoto <[email protected]> * update Signed-off-by: Kosuke Morimoto <[email protected]> * fix test Signed-off-by: Kosuke Morimoto <[email protected]> * add unit test Signed-off-by: Kosuke Morimoto <[email protected]> * style: format code with Gofumpt and Prettier This commit fixes the style issues introduced in de3ef81 according to the output from Gofumpt and Prettier. Details: #2584 * add codecov.yml to ignore generated code Signed-off-by: Kosuke Morimoto <[email protected]> * make format Signed-off-by: Kosuke Morimoto <[email protected]> * fix conflict Signed-off-by: Kosuke Morimoto <[email protected]> * fix Signed-off-by: Kosuke Morimoto <[email protected]> * style: format code with Gofumpt and Prettier This commit fixes the style issues introduced in 1d43dbb according to the output from Gofumpt and Prettier. Details: #2584 * fix Signed-off-by: Kosuke Morimoto <[email protected]> * format Signed-off-by: Kosuke Morimoto <[email protected]> * add codecov ignore Signed-off-by: Kosuke Morimoto <[email protected]> * add unit test Signed-off-by: Kosuke Morimoto <[email protected]> --------- Signed-off-by: Kosuke Morimoto <[email protected]> Co-authored-by: deepsource-autofix[bot] <62050782+deepsource-autofix[bot]@users.noreply.github.com>
Deploying vald with
|
| Latest commit: |
bf54494
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://7a07f736.vald.pages.dev |
| Branch Preview URL: | https://backport-release-v1-7-featur-6hhp.vald.pages.dev |
|
Important Review skippedReview was skipped due to path filters Files ignored due to path filters (2)
You can disable this status message by setting the WalkthroughWalkthroughThe changes introduce enhancements to the API documentation and functionality related to index properties across several components. New sections, methods, and data structures have been added to improve the management and retrieval of index properties through both gRPC and RESTful interfaces. Additionally, updates to Go module versions have been made across multiple files, ensuring consistency in the development environment. Changes
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 (
|
|
[WARNING:CONFLICT] You may require to fix the conflict. Please check. |
|
[CHATOPS:HELP] ChatOps commands.
|
github-actions
bot
added
area/manager/index
area/filter/ingress
area/filter/egress
area/internal
area/agent/sidecar
area/tools/cli/loadtest
area/gateway/lb
area/gateway/filter
area/agent/core/ngt
area/gateway/mirror
area/index/job/correction
area/index/job/creation
area/index/job/save
area/index/job/readreplica
size/XXXL
priority/low
labels
![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
Outside diff range, codebase verification and nitpick comments (5)
internal/core/algorithm/ngt/ngt.go (1)
162-197: DefinePropertystruct.The
Propertystruct encapsulates various NGT index properties. Ensure all fields are necessary and consider adding comments for clarity.Property struct { + // Dimension represents the dimensionality of the index. Dimension int32 + // ThreadPoolSize specifies the size of the thread pool. ThreadPoolSize int32 // ... (add comments for other fields as needed) }apis/docs/v1/docs.md (3)
328-368: Add descriptions for fields inInfo.Index.Property.The fields in this section are clearly listed, but adding descriptions would enhance understanding and usability of the documentation.
371-378: Add descriptions for fields inInfo.Index.PropertyDetail.Providing descriptions for the fields would improve the clarity and usability of the documentation.
381-387: Add descriptions for fields inInfo.Index.PropertyDetail.DetailsEntry.Including descriptions for the fields would enhance the documentation's clarity and usability.
internal/core/algorithm/ngt/ngt_test.go (1)
4769-4885: LGTM! Minor readability improvement suggested.The test function
Test_ngt_Propertyis well-structured and comprehensive. It effectively validates the properties of the NGT structure.For improved readability, consider using a loop to iterate over the properties being checked in the
checkFunc. This can reduce repetitive code and make it easier to add additional checks in the future.Example:
checkFunc := func(w want, p *Property, err error) error { if err := defaultCheckFunc(w, p, err); err != nil { return err } properties := map[string]interface{}{ "Dimension": p.Dimension, "ObjectType": p.ObjectType, "DistanceType": p.DistanceType, "IndexType": p.IndexType, "DatabaseType": p.DatabaseType, "GraphType": p.GraphType, } for name, got := range properties { want := reflect.ValueOf(w.want).Elem().FieldByName(name).Interface() if !reflect.DeepEqual(got, want) { return errors.Errorf("got_%s: \"%v\", want_%s: \"%v\"", name, got, name, want) } } return nil }
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (9)
apis/grpc/v1/agent/sidecar/sidecar_vtproto.pb.gois excluded by!**/*.pb.go,!**/*.pb.goapis/grpc/v1/payload/payload.pb.gois excluded by!**/*.pb.go,!**/*.pb.goapis/grpc/v1/payload/payload.pb.json.gois excluded by!**/*.pb.json.goapis/grpc/v1/payload/payload_vtproto.pb.gois excluded by!**/*.pb.go,!**/*.pb.goapis/grpc/v1/vald/index.pb.gois excluded by!**/*.pb.go,!**/*.pb.goapis/grpc/v1/vald/index_vtproto.pb.gois excluded by!**/*.pb.go,!**/*.pb.goexample/client/go.sumis excluded by!**/*.sumgo.sumis excluded by!**/*.sumrust/Cargo.lockis excluded by!**/*.lock
Files selected for processing (25)
- apis/docs/v1/docs.md (3 hunks)
- apis/grpc/v1/vald/vald.go (1 hunks)
- apis/proto/v1/payload/payload.proto (1 hunks)
- apis/proto/v1/vald/index.proto (1 hunks)
- apis/swagger/v1/vald/index.swagger.json (2 hunks)
- codecov.yml (1 hunks)
- example/client/go.mod (2 hunks)
- example/client/go.mod.default (1 hunks)
- go.mod (16 hunks)
- hack/go.mod.default (1 hunks)
- internal/client/v1/client/vald/vald.go (2 hunks)
- internal/core/algorithm/ngt/ngt.go (7 hunks)
- internal/core/algorithm/ngt/ngt_test.go (1 hunks)
- internal/core/algorithm/ngt/option.go (1 hunks)
- internal/net/http/client/client_test.go (1 hunks)
- pkg/agent/core/ngt/handler/grpc/index.go (1 hunks)
- pkg/agent/core/ngt/handler/grpc/index_test.go (2 hunks)
- pkg/agent/core/ngt/service/ngt.go (2 hunks)
- pkg/gateway/lb/handler/grpc/handler.go (1 hunks)
- rust/bin/agent/src/handler/index.rs (1 hunks)
- rust/libs/proto/src/filter.egress.v1.tonic.rs (2 hunks)
- rust/libs/proto/src/filter.ingress.v1.tonic.rs (2 hunks)
- rust/libs/proto/src/payload.v1.rs (1 hunks)
- rust/libs/proto/src/vald.v1.tonic.rs (9 hunks)
- versions/GO_VERSION (1 hunks)
Files skipped from review due to trivial changes (5)
- codecov.yml
- example/client/go.mod.default
- rust/libs/proto/src/filter.egress.v1.tonic.rs
- rust/libs/proto/src/filter.ingress.v1.tonic.rs
- versions/GO_VERSION
Additional comments not posted (29)
example/client/go.mod (2)
3-3: Upgrade to Go 1.23.0.The Go version has been updated from 1.22.6 to 1.23.0. Ensure compatibility with the rest of the codebase and verify if any new language features or changes need to be addressed.
14-16: Dependency updates forgoogle.golang.org/genproto.The
google.golang.org/genprotodependencies have been updated to a newer version. Verify that these updates do not introduce breaking changes and are compatible with the rest of the codebase.apis/proto/v1/vald/index.proto (1)
49-52: Addition ofIndexPropertyRPC method.The
IndexPropertymethod has been added to theIndexservice, providing a way to retrieve index properties. Ensure that the corresponding server-side implementation and client-side usage are correctly handled.apis/grpc/v1/vald/vald.go (1)
142-142: Addition ofIndexPropertyRPCNameis appropriate.The new constant
IndexPropertyRPCNameis added consistently with the existing pattern of RPC name constants.internal/net/http/client/client_test.go (1)
49-49: IgnoringdialsInProgressintransportComparatoris valid.Adding
"dialsInProgress"to the ignored fields in the comparator is a sensible change, as it likely represents a transient state that should not affect test outcomes.pkg/agent/core/ngt/handler/grpc/index.go (1)
244-262: Addition ofIndexPropertymethod is well-implemented.The new
IndexPropertymethod follows the existing patterns for error handling and tracing, making it a consistent and well-structured addition to the codebase.internal/core/algorithm/ngt/option.go (1)
112-112: LGTM! Initialization change enhances clarity.The initialization of
dtoDistanceNoneprovides a clear starting state, which is beneficial for the subsequent logic.apis/swagger/v1/vald/index.swagger.json (2)
169-300: New object definitionsIndexPropertyandIndexPropertyDetailadded.The new object definitions provide a structured way to represent index properties, enhancing the API's functionality.
57-77: New endpoint/index/propertyadded.The addition of the
/index/propertyendpoint enhances the API by providing a way to retrieve index properties.Ensure that this endpoint is properly integrated and tested within the system.
Run the following script to check for the integration of the new endpoint in the codebase:
apis/proto/v1/payload/payload.proto (2)
594-630: New message typePropertyadded.The
Propertymessage type encapsulates detailed index properties, enhancing the data model.
632-635: New message typePropertyDetailadded.The
PropertyDetailmessage type allows for flexible handling of multiple index properties using a map structure.Ensure that these new message types are correctly utilized in the codebase.
Run the following script to check for the usage of the new message types:
Verification successful
New message types
PropertyandPropertyDetailare correctly utilized.The
PropertyandPropertyDetailmessage types are integrated into various parts of the codebase, including RPC calls and service definitions, confirming their correct utilization.
PropertyDetailis used in RPC calls such asIndexProperty.Propertyis referenced in multiple files, including C++ and Rust code.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `Property` and `PropertyDetail` message types. # Test: Search for the message types usage. Expect: Occurrences of the new message types in the codebase. rg --type proto 'Property|PropertyDetail'Length of output: 73
Script:
#!/bin/bash # Description: Verify the usage of `Property` and `PropertyDetail` message types in .proto files. # Find all .proto files and search for the message types usage. fd --extension proto --exec rg 'Property|PropertyDetail' {}Length of output: 276
Script:
#!/bin/bash # Description: Verify the usage of `Property` and `PropertyDetail` message types in the entire codebase. # Search for the message types usage in all files. rg 'Property|PropertyDetail'Length of output: 24464
hack/go.mod.default (2)
3-3: Update Go version to 1.23.0.The Go version has been updated from 1.22.6 to 1.23.0. Ensure compatibility with the new Go version across the codebase.
5-5: Upgrade dependencies.The
replacedirective indicates that several dependencies are set to upgrade. Ensure these upgrades are compatible with the rest of the codebase and do not introduce breaking changes.internal/core/algorithm/ngt/ngt.go (4)
90-91: AddGetPropertymethod to NGT interface.The
GetPropertymethod has been added to the NGT interface to retrieve index properties. Ensure this method is properly tested and documented.
226-239: Define new types for index properties.New types
indexType,databaseType,objectAlignment,seedType, andgraphTypehave been defined to enhance type safety. Ensure these types are used consistently throughout the codebase.
399-463: Implement stringer methods for new types.Stringer methods have been implemented for the new types, providing human-readable representations. Ensure these methods cover all possible values and consider adding tests.
1160-1199: ImplementGetPropertymethod.The
GetPropertymethod retrieves NGT index properties using C API calls. Ensure error handling is robust and consider adding unit tests for this method.go.mod (2)
3-3: Update Go version to 1.23.0.The Go version has been updated from 1.22.6 to 1.23.0. Ensure compatibility with the new Go version across the codebase.
Line range hint
6-321: Upgrade dependencies.Several dependencies have been updated to newer versions. Ensure these upgrades are compatible with the rest of the codebase and do not introduce breaking changes.
internal/client/v1/client/vald/vald.go (2)
831-851: LGTM! TheIndexPropertymethod is well-implemented.The method correctly handles gRPC calls with context tracing and round-robin strategy for load balancing.
1295-1305: LGTM! TheIndexPropertymethod insingleClientis correctly implemented.The method maintains consistency with other methods in the
singleClientstructure.rust/libs/proto/src/payload.v1.rs (2)
959-1031: LGTM! ThePropertystruct is well-defined.The fields are appropriately annotated for serialization using the Prost library.
1032-1038: LGTM! ThePropertyDetailstruct is correctly implemented.The use of a
HashMapis appropriate for organizing and retrieving multiple indexing properties.pkg/agent/core/ngt/service/ngt.go (1)
2048-2089: LGTM! TheIndexPropertymethod is well-implemented.The method efficiently retrieves and constructs a detailed structure of index properties.
pkg/agent/core/ngt/handler/grpc/index_test.go (1)
2623-2623: Good use ofgoleak.VerifyNone.The use of
goleak.VerifyNonehelps ensure that there are no goroutine leaks during the test execution.apis/docs/v1/docs.md (1)
1635-1635: Addition ofIndexPropertyRPC method is appropriate.The
IndexPropertymethod is a valuable addition to the RPC section, aligning with the API's functionality.pkg/gateway/lb/handler/grpc/handler.go (1)
4065-4179: Verify error handling and consider performance optimizations.The
IndexPropertyfunction is well-structured with robust error handling and tracing capabilities. However, ensure that all potential errors are correctly handled, especially in the context of concurrent operations. Additionally, consider optimizing the performance by evaluating the use of channels and mutexes, particularly in high-concurrency scenarios.Run the following script to verify the error handling and concurrency management in the codebase:
Verification successful
Error handling and concurrency management are correctly implemented.
The
IndexPropertyfunction inpkg/gateway/lb/handler/grpc/handler.godemonstrates robust error handling and appropriate use of concurrency mechanisms. The function effectively uses a mutex to manage concurrent access to shared resources and handles various error scenarios comprehensively.
- Location:
pkg/gateway/lb/handler/grpc/handler.go, lines 4065-4179Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify error handling and concurrency management in the `IndexProperty` function. # Test: Search for error handling and mutex usage. Expect: Correct usage and handling. rg --type go -A 5 'func (s \*server) IndexProperty'Length of output: 53
Script:
#!/bin/bash # Description: Verify error handling and concurrency management in the `IndexProperty` function. # Test: Search for the complete implementation of the `IndexProperty` function. ast-grep --lang go --pattern 'func (s *server) IndexProperty($_, $_) ($_, $_) { $$$ }'Length of output: 10833
rust/libs/proto/src/vald.v1.tonic.rs (2)
1836-1864: Addition ofindex_propertymethod inindex_client.The addition of the
index_propertymethod is well-structured and follows the existing pattern for gRPC client methods. It includes proper error handling and codec configuration.
1908-1918: Addition ofindex_propertymethod inindex_server.The
index_propertymethod is correctly defined and aligns with the structure of other server methods. It provides a clear interface for implementing the retrieval of index properties.
| async fn index_property( | ||
| &self, | ||
| request: tonic::Request<Empty>, | ||
| ) -> std::result::Result<tonic::Response<info::index::PropertyDetail>, tonic::Status> { | ||
| todo!() |
There was a problem hiding this comment.
Implement the index_property function.
The index_property function is currently a placeholder. Implement the logic to handle RPC requests for retrieving index properties.
Would you like assistance in implementing this function or opening a GitHub issue to track this task?
| }, | ||
| want: want{ | ||
| wantRes: &payload.Info_Index_PropertyDetail{ | ||
| Details: map[string]*payload.Info_Index_Property{ | ||
| name: {}, | ||
| }, | ||
| }, | ||
| }, | ||
| checkFunc: func(w want, gotRes *payload.Info_Index_PropertyDetail, err error) error { | ||
| if !errors.Is(err, w.err) { | ||
| return errors.Errorf("got_error: \"%#v\",\n\t\t\t\twant: \"%#v\"", err, w.err) | ||
| } | ||
| if val, ok := gotRes.Details[name]; ok { | ||
| gotType := reflect.TypeOf(val) | ||
| wantType := reflect.TypeOf(&payload.Info_Index_Property{}) | ||
| if gotType != wantType { | ||
| return errors.Errorf("got_type: \"%s\", want_type: \"%s\"", gotType, wantType) | ||
| } | ||
| } else { | ||
| return errors.Errorf("do not exists key: %s", name) | ||
| } | ||
| return nil | ||
| }, | ||
| beforeFunc: func(t *testing.T, args args) { | ||
| t.Helper() | ||
| }, | ||
| afterFunc: func(t *testing.T, args args) { | ||
| t.Helper() | ||
| }, | ||
| }, | ||
|
|
||
| // TODO test cases | ||
| /* | ||
| func() test { | ||
| return test { | ||
| name: "test_case_2", | ||
| args: args { | ||
| ctx:nil, | ||
| in1:nil, | ||
| }, | ||
| fields: fields { | ||
| name:"", | ||
| ip:"", | ||
| ngt:nil, | ||
| eg:nil, | ||
| streamConcurrency:0, | ||
| UnimplementedAgentServer:nil, | ||
| UnimplementedValdServer:nil, | ||
| }, | ||
| want: want{}, | ||
| checkFunc: defaultCheckFunc, | ||
| beforeFunc: func(t *testing.T, args args) { | ||
| t.Helper() | ||
| }, | ||
| afterFunc: func(t *testing.T, args args) { | ||
| t.Helper() | ||
| }, | ||
| } | ||
| }(), | ||
| */ | ||
| } | ||
|
|
||
| for _, tc := range tests { | ||
| test := tc | ||
| t.Run(test.name, func(tt *testing.T) { | ||
| tt.Parallel() | ||
|
|
||
| ctx, cancel := context.WithCancel(context.Background()) | ||
| defer cancel() | ||
|
|
||
| defer goleak.VerifyNone(tt, goleak.IgnoreCurrent()) | ||
| if test.beforeFunc != nil { | ||
| test.beforeFunc(tt, test.args) | ||
| } | ||
| if test.afterFunc != nil { | ||
| defer test.afterFunc(tt, test.args) | ||
| } | ||
| checkFunc := test.checkFunc | ||
| if test.checkFunc == nil { | ||
| checkFunc = defaultCheckFunc | ||
| } | ||
| eg, _ := errgroup.New(ctx) | ||
| ngt, err := service.New(test.fields.svcCfg, append(test.fields.svcOpts, service.WithErrGroup(eg))...) | ||
| if err != nil { | ||
| tt.Errorf("failed to init ngt service, error = %v", err) | ||
| } | ||
| s, err := New( | ||
| WithErrGroup(eg), | ||
| WithNGT(ngt), | ||
| WithIP(ip), | ||
| WithName(name), | ||
| ) | ||
|
|
||
| gotRes, err := s.IndexProperty(ctx, test.args.in) | ||
| if err := checkFunc(test.want, gotRes, err); err != nil { | ||
| tt.Errorf("error = %v", err) | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Consider adding more test cases for IndexProperty.
The current test function only includes one test case. Consider adding more cases to cover different scenarios and edge cases for the IndexProperty method.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## release/v1.7 #2588 +/- ##
===============================================
Coverage ? 24.55%
===============================================
Files ? 530
Lines ? 45505
Branches ? 0
===============================================
Hits ? 11172
Misses ? 33586
Partials ? 747 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range, codebase verification and nitpick comments (9)
pkg/agent/core/ngt/handler/grpc/index.go (1)
253-255: Handle errors with detailed logging.The error returned from
s.ngt.IndexProperty()is not logged. Consider adding logging for better traceability.Apply this diff to add error logging:
if err != nil { + log.Error(err) return nil, err }apis/swagger/v1/vald/index.swagger.json (1)
169-299: Review property descriptions for completeness.Some properties in the
IndexPropertyschema lack descriptions. Consider adding descriptions for all properties to enhance clarity.apis/proto/v1/payload/payload.proto (1)
595-630: Add field comments for clarity.Consider adding comments for each field in the
Propertymessage to describe their purpose and usage.pkg/agent/core/ngt/handler/grpc/index_test.go (1)
2477-2652: LGTM! Consider adding more test cases.The test function
Test_server_IndexPropertyis well-structured and correctly tests theIndexPropertymethod. However, it currently includes only one test case. Consider adding more test cases to cover additional scenarios and edge cases.apis/docs/v1/docs.md (2)
326-368: Add descriptions for each field inInfo.Index.Property.The section outlines various attributes associated with index properties, but it lacks descriptions for each field. Adding descriptions will improve clarity and usability.
379-387: Add descriptions for fields inInfo.Index.PropertyDetail.DetailsEntry.The section defines the structure for individual detail entries but lacks descriptions for the fields. Adding descriptions will enhance understanding.
pkg/gateway/lb/handler/grpc/handler.go (1)
4065-4067: Consider Adding Documentation forIndexProperty.Adding a brief comment explaining the purpose and functionality of the
IndexPropertymethod would improve code readability and maintainability.// IndexProperty retrieves and aggregates index property details from multiple agents.rust/libs/proto/src/vald.v1.tonic.rs (2)
1836-1864: Incomplete Implementation Detected forindex_propertyMethodThe
index_propertymethod is defined and referenced in the codebase, but its implementation inrust/bin/agent/src/handler/index.rsis incomplete, marked withtodo!(). Ensure this method is fully implemented and integrated into the codebase.
rust/bin/agent/src/handler/index.rs: Implementation is incomplete.Analysis chain
LGTM! Verify the method usage in the codebase.
The
index_propertymethod is correctly implemented. Ensure that all calls to this method are correctly integrated into the codebase.Run the following script to verify the method usage:
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `index_property` are correctly integrated. # Test: Search for the function usage. Expect: Occurrences of the new method. rg --type rust -A 5 $'index_property'Length of output: 1981
1908-1918: Incomplete Integration ofindex_propertyMethodThe
index_propertymethod is defined but not yet fully integrated or called within the codebase. It appears in the following locations:
- Defined with
todo!()inrust/bin/agent/src/handler/index.rs, indicating incomplete implementation.- Part of a trait implementation in
rust/libs/proto/src/vald.v1.tonic.rs.Ensure that this method is fully implemented and properly integrated into the application.
Analysis chain
LGTM! Verify the method usage in the codebase.
The
index_propertymethod is correctly implemented. Ensure that all calls to this method are correctly integrated into the codebase.Run the following script to verify the method usage:
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `index_property` are correctly integrated. # Test: Search for the function usage. Expect: Occurrences of the new method. rg --type rust -A 5 $'index_property'Length of output: 1981
Script:
#!/bin/bash # Description: Search for calls to the `index_property` method to verify its integration. # Test: Search for calls to the `index_property` method. rg --type rust 'index_property\('Length of output: 365
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (9)
apis/grpc/v1/agent/sidecar/sidecar_vtproto.pb.gois excluded by!**/*.pb.go,!**/*.pb.goapis/grpc/v1/payload/payload.pb.gois excluded by!**/*.pb.go,!**/*.pb.goapis/grpc/v1/payload/payload.pb.json.gois excluded by!**/*.pb.json.goapis/grpc/v1/payload/payload_vtproto.pb.gois excluded by!**/*.pb.go,!**/*.pb.goapis/grpc/v1/vald/index.pb.gois excluded by!**/*.pb.go,!**/*.pb.goapis/grpc/v1/vald/index_vtproto.pb.gois excluded by!**/*.pb.go,!**/*.pb.goexample/client/go.sumis excluded by!**/*.sumgo.sumis excluded by!**/*.sumrust/Cargo.lockis excluded by!**/*.lock
Files selected for processing (25)
- apis/docs/v1/docs.md (3 hunks)
- apis/grpc/v1/vald/vald.go (1 hunks)
- apis/proto/v1/payload/payload.proto (1 hunks)
- apis/proto/v1/vald/index.proto (1 hunks)
- apis/swagger/v1/vald/index.swagger.json (2 hunks)
- codecov.yml (1 hunks)
- example/client/go.mod (2 hunks)
- example/client/go.mod.default (1 hunks)
- go.mod (16 hunks)
- hack/go.mod.default (1 hunks)
- internal/client/v1/client/vald/vald.go (2 hunks)
- internal/core/algorithm/ngt/ngt.go (7 hunks)
- internal/core/algorithm/ngt/ngt_test.go (1 hunks)
- internal/core/algorithm/ngt/option.go (1 hunks)
- internal/net/http/client/client_test.go (1 hunks)
- pkg/agent/core/ngt/handler/grpc/index.go (1 hunks)
- pkg/agent/core/ngt/handler/grpc/index_test.go (2 hunks)
- pkg/agent/core/ngt/service/ngt.go (2 hunks)
- pkg/gateway/lb/handler/grpc/handler.go (1 hunks)
- rust/bin/agent/src/handler/index.rs (1 hunks)
- rust/libs/proto/src/filter.egress.v1.tonic.rs (2 hunks)
- rust/libs/proto/src/filter.ingress.v1.tonic.rs (2 hunks)
- rust/libs/proto/src/payload.v1.rs (1 hunks)
- rust/libs/proto/src/vald.v1.tonic.rs (9 hunks)
- versions/GO_VERSION (1 hunks)
Files skipped from review due to trivial changes (5)
- codecov.yml
- internal/core/algorithm/ngt/option.go
- rust/libs/proto/src/filter.egress.v1.tonic.rs
- rust/libs/proto/src/filter.ingress.v1.tonic.rs
- versions/GO_VERSION
Additional context used
GitHub Check: codecov/patch
internal/core/algorithm/ngt/ngt.go
[warning] 399-404: internal/core/algorithm/ngt/ngt.go#L399-L404
Added lines #L399 - L404 were not covered by tests
[warning] 406-406: internal/core/algorithm/ngt/ngt.go#L406
Added line #L406 was not covered by tests
[warning] 409-414: internal/core/algorithm/ngt/ngt.go#L409-L414
Added lines #L409 - L414 were not covered by tests
[warning] 416-416: internal/core/algorithm/ngt/ngt.go#L416
Added line #L416 was not covered by tests
[warning] 419-424: internal/core/algorithm/ngt/ngt.go#L419-L424
Added lines #L419 - L424 were not covered by tests
[warning] 426-426: internal/core/algorithm/ngt/ngt.go#L426
Added line #L426 was not covered by tests
[warning] 429-438: internal/core/algorithm/ngt/ngt.go#L429-L438
Added lines #L429 - L438 were not covered by tests
[warning] 440-440: internal/core/algorithm/ngt/ngt.go#L440
Added line #L440 was not covered by tests
[warning] 443-460: internal/core/algorithm/ngt/ngt.go#L443-L460
Added lines #L443 - L460 were not covered by tests
[warning] 462-462: internal/core/algorithm/ngt/ngt.go#L462
Added line #L462 was not covered by tests
Additional comments not posted (25)
example/client/go.mod.default (1)
3-3: Go version update acknowledged.The Go version has been updated to 1.23.0. Ensure compatibility with the updated version.
example/client/go.mod (1)
3-3: Go version and dependency updates acknowledged.The Go version has been updated to 1.23.0, and several dependencies have been updated. Ensure compatibility with the updated versions.
Also applies to: 14-16
apis/proto/v1/vald/index.proto (1)
49-52: New RPC methodIndexPropertyadded.The addition of the
IndexPropertyRPC method enhances the service's functionality by providing a way to retrieve index properties.rust/bin/agent/src/handler/index.rs (1)
80-86: Documentation and structure look good.The documentation and function structure are consistent with the rest of the trait implementation.
apis/grpc/v1/vald/vald.go (1)
142-142: Addition ofIndexPropertyRPCNameis approved.The new constant aligns with the existing pattern for RPC names and enhances the functionality of the RPC client.
internal/net/http/client/client_test.go (1)
49-49: Update totransportComparatoris approved.The addition of
"dialsInProgress"to the ignored fields enhances test robustness by excluding a non-essential field from comparisons.apis/swagger/v1/vald/index.swagger.json (2)
57-76: Ensure operation ID consistency.The operation ID
Index_IndexPropertyshould be consistent with naming conventions used in other endpoints. Verify that it aligns with the project's standards.
301-312: Ensure schema title consistency.The title "Represents index Properties for each Agents" should be consistent with other schema titles. Verify that it aligns with the project's naming conventions.
apis/proto/v1/payload/payload.proto (1)
632-635: Ensure map key consistency.The map key in
PropertyDetailshould be consistent with other map keys used in the protocol. Verify that it aligns with the project's standards.hack/go.mod.default (1)
3-3: LGTM: Go version and dependency updates.The updates to the Go version and dependencies are appropriate and align with the project's needs.
Also applies to: 6-6, 8-8, 11-11, 12-12, 13-13, 15-15, 17-17
internal/core/algorithm/ngt/ngt.go (2)
90-91: LGTM: NewGetPropertymethod andPropertystruct.The addition of the
GetPropertymethod andPropertystruct enhances the functionality by providing structured access to NGT properties.Also applies to: 1160-1200
399-407: Add tests for stringer methods.The stringer methods for the new types are not covered by tests. Consider adding unit tests to ensure they return the expected string representations.
Would you like assistance in generating the test cases for these methods?
Also applies to: 409-417, 419-427, 429-441, 443-463
Tools
GitHub Check: codecov/patch
[warning] 399-404: internal/core/algorithm/ngt/ngt.go#L399-L404
Added lines #L399 - L404 were not covered by tests
[warning] 406-406: internal/core/algorithm/ngt/ngt.go#L406
Added line #L406 was not covered by testsgo.mod (1)
3-3: LGTM: Go version and dependency updates.The updates to the Go version and dependencies are appropriate and align with the project's needs.
Also applies to: 6-6, 8-8, 11-11, 12-12, 13-13, 15-15, 17-17, 39-39, 41-42, 48-69, 240-240, 296-296, 298-299, 307-307, 314-314, 317-317, 319-321
internal/client/v1/client/vald/vald.go (2)
831-851: New Method Addition:IndexPropertyinclient.The
IndexPropertymethod is well-structured, consistent with other methods in the file, and effectively manages tracing and error handling.
1295-1305: New Method Addition:IndexPropertyinsingleClient.The
IndexPropertymethod is consistent with the existing methods, effectively managing tracing and error handling.rust/libs/proto/src/payload.v1.rs (2)
959-1031: New Struct Addition:Property.The
Propertystruct is well-defined with appropriate data types for each field, aligning with the intended use.
1032-1038: New Struct Addition:PropertyDetail.The
PropertyDetailstruct is well-structured, effectively allowing for the management of multiple indexing configurations.pkg/agent/core/ngt/service/ngt.go (1)
2048-2089: New Method Addition:IndexPropertyinngt.The
IndexPropertymethod is well-implemented, with proper error handling and data mapping, aligning with the intended functionality.apis/docs/v1/docs.md (2)
369-378: SectionInfo.Index.PropertyDetailis clear and concise.The section accurately represents index properties for each agent.
1635-1635: Addition ofIndexPropertyto RPC definitions is clear and consistent.The addition aligns well with existing RPC definitions.
internal/core/algorithm/ngt/ngt_test.go (1)
4769-4885: Ensure comprehensive test coverage for all fields.The test function
Test_ngt_Propertyis well-structured and verifies multiple fields of thePropertystruct. Ensure that all relevant fields of thePropertystruct are covered in the test cases to maintain comprehensive test coverage.pkg/gateway/lb/handler/grpc/handler.go (4)
4068-4073: Ensure Proper Span Closure.The span is being checked for
nilbefore callingEnd(), which is good practice. Ensure this pattern is consistently applied across all methods.
4074-4078: Initialize Detail with Capacity.The
Detailsmap is initialized with a capacity equal to the number of agents. This is a good practice to optimize memory allocation.
4080-4140: Handle Errors in Goroutine Properly.The goroutine uses a channel to communicate errors back to the main function. Ensure that the channel is closed properly and that all potential errors are captured.
4146-4178: Comprehensive Error Handling.The method includes comprehensive error handling with detailed tracing and logging. This is a good practice for maintaining robust and debuggable code.
| #[doc = " Represent the RPC to get the index property.\n"] | ||
| async fn index_property( | ||
| &self, | ||
| request: tonic::Request<Empty>, | ||
| ) -> std::result::Result<tonic::Response<info::index::PropertyDetail>, tonic::Status> { | ||
| todo!() | ||
| } |
There was a problem hiding this comment.
Implement the index_property function.
The function is currently a placeholder with todo!(). Consider implementing the function to handle RPC requests for retrieving index properties.
Do you want me to help with the implementation or open a GitHub issue to track this task?
| func (s *server) IndexProperty( | ||
| ctx context.Context, _ *payload.Empty, | ||
| ) (res *payload.Info_Index_PropertyDetail, err error) { | ||
| _, span := trace.StartSpan(ctx, apiName+".IndexStatisticsDetail") | ||
| defer func() { | ||
| if span != nil { | ||
| span.End() | ||
| } | ||
| }() | ||
| prop, err := s.ngt.IndexProperty() | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return &payload.Info_Index_PropertyDetail{ | ||
| Details: map[string]*payload.Info_Index_Property{ | ||
| s.name: prop, | ||
| }, | ||
| }, nil | ||
| } |
There was a problem hiding this comment.
Ensure consistency in tracing span naming.
The tracing span name is set to IndexStatisticsDetail, which seems inconsistent with the method name IndexProperty. Consider updating it for clarity.
Apply this diff to correct the span name:
_, span := trace.StartSpan(ctx, apiName+".IndexStatisticsDetail")
+_, span := trace.StartSpan(ctx, apiName+".IndexProperty")Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (s *server) IndexProperty( | |
| ctx context.Context, _ *payload.Empty, | |
| ) (res *payload.Info_Index_PropertyDetail, err error) { | |
| _, span := trace.StartSpan(ctx, apiName+".IndexStatisticsDetail") | |
| defer func() { | |
| if span != nil { | |
| span.End() | |
| } | |
| }() | |
| prop, err := s.ngt.IndexProperty() | |
| if err != nil { | |
| return nil, err | |
| } | |
| return &payload.Info_Index_PropertyDetail{ | |
| Details: map[string]*payload.Info_Index_Property{ | |
| s.name: prop, | |
| }, | |
| }, nil | |
| } | |
| func (s *server) IndexProperty( | |
| ctx context.Context, _ *payload.Empty, | |
| ) (res *payload.Info_Index_PropertyDetail, err error) { | |
| _, span := trace.StartSpan(ctx, apiName+".IndexProperty") | |
| defer func() { | |
| if span != nil { | |
| span.End() | |
| } | |
| }() | |
| prop, err := s.ngt.IndexProperty() | |
| if err != nil { | |
| return nil, err | |
| } | |
| return &payload.Info_Index_PropertyDetail{ | |
| Details: map[string]*payload.Info_Index_Property{ | |
| s.name: prop, | |
| }, | |
| }, nil | |
| } |
| prop, err = vc.IndexProperty(sctx, new(payload.Empty), copts...) | ||
| if err != nil { | ||
| var ( | ||
| attrs trace.Attributes | ||
| st *status.Status | ||
| msg string | ||
| code codes.Code | ||
| ) | ||
| switch { | ||
| case errors.Is(err, context.Canceled), errors.Is(err, errors.ErrRPCCallFailed(target, context.Canceled)): | ||
| attrs = trace.StatusCodeCancelled( | ||
| errdetails.ValdGRPCResourceTypePrefix + "/vald.v1." + vald.IndexPropertyRPCName + ".BroadCast/" + target + " canceled: " + err.Error()) | ||
| code = codes.Canceled | ||
| case errors.Is(err, context.DeadlineExceeded), errors.Is(err, errors.ErrRPCCallFailed(target, context.DeadlineExceeded)): | ||
| attrs = trace.StatusCodeDeadlineExceeded( | ||
| errdetails.ValdGRPCResourceTypePrefix + "/vald.v1." + vald.IndexPropertyRPCName + ".BroadCast/" + target + " deadline_exceeded: " + err.Error()) | ||
| code = codes.DeadlineExceeded | ||
| default: | ||
| st, msg, err = status.ParseError(err, codes.NotFound, "error "+vald.IndexPropertyRPCName+" API", | ||
| &errdetails.ResourceInfo{ | ||
| ResourceType: errdetails.ValdGRPCResourceTypePrefix + "/vald.v1." + vald.IndexPropertyRPCName + ".BroadCast/" + target, | ||
| ResourceName: fmt.Sprintf("%s: %s(%s) to %s", apiName, s.name, s.ip, target), | ||
| }) | ||
| if st != nil { | ||
| code = st.Code() | ||
| } else { | ||
| code = codes.NotFound | ||
| } | ||
| attrs = trace.FromGRPCStatus(code, msg) | ||
| } | ||
| if sspan != nil { | ||
| sspan.RecordError(err) | ||
| sspan.SetAttributes(attrs...) | ||
| sspan.SetStatus(trace.StatusError, err.Error()) | ||
| } | ||
| if err != nil && st != nil && code != codes.Canceled && code != codes.DeadlineExceeded && code != codes.InvalidArgument && code != codes.NotFound && code != codes.OK && code != codes.Unimplemented { | ||
| return err | ||
| } | ||
| return nil | ||
| } | ||
| if prop != nil { | ||
| mu.Lock() | ||
| for key, value := range prop.Details { | ||
| detail.Details[target+"-"+key] = value | ||
| } | ||
| mu.Unlock() |
There was a problem hiding this comment.
Check for Nil Property Before Accessing Details.
Before accessing prop.Details, ensure prop is not nil to avoid potential runtime panics.
if prop != nil {
+ if prop.Details != nil {
mu.Lock()
for key, value := range prop.Details {
detail.Details[target+"-"+key] = value
}
mu.Unlock()
+ }
}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| prop, err = vc.IndexProperty(sctx, new(payload.Empty), copts...) | |
| if err != nil { | |
| var ( | |
| attrs trace.Attributes | |
| st *status.Status | |
| msg string | |
| code codes.Code | |
| ) | |
| switch { | |
| case errors.Is(err, context.Canceled), errors.Is(err, errors.ErrRPCCallFailed(target, context.Canceled)): | |
| attrs = trace.StatusCodeCancelled( | |
| errdetails.ValdGRPCResourceTypePrefix + "/vald.v1." + vald.IndexPropertyRPCName + ".BroadCast/" + target + " canceled: " + err.Error()) | |
| code = codes.Canceled | |
| case errors.Is(err, context.DeadlineExceeded), errors.Is(err, errors.ErrRPCCallFailed(target, context.DeadlineExceeded)): | |
| attrs = trace.StatusCodeDeadlineExceeded( | |
| errdetails.ValdGRPCResourceTypePrefix + "/vald.v1." + vald.IndexPropertyRPCName + ".BroadCast/" + target + " deadline_exceeded: " + err.Error()) | |
| code = codes.DeadlineExceeded | |
| default: | |
| st, msg, err = status.ParseError(err, codes.NotFound, "error "+vald.IndexPropertyRPCName+" API", | |
| &errdetails.ResourceInfo{ | |
| ResourceType: errdetails.ValdGRPCResourceTypePrefix + "/vald.v1." + vald.IndexPropertyRPCName + ".BroadCast/" + target, | |
| ResourceName: fmt.Sprintf("%s: %s(%s) to %s", apiName, s.name, s.ip, target), | |
| }) | |
| if st != nil { | |
| code = st.Code() | |
| } else { | |
| code = codes.NotFound | |
| } | |
| attrs = trace.FromGRPCStatus(code, msg) | |
| } | |
| if sspan != nil { | |
| sspan.RecordError(err) | |
| sspan.SetAttributes(attrs...) | |
| sspan.SetStatus(trace.StatusError, err.Error()) | |
| } | |
| if err != nil && st != nil && code != codes.Canceled && code != codes.DeadlineExceeded && code != codes.InvalidArgument && code != codes.NotFound && code != codes.OK && code != codes.Unimplemented { | |
| return err | |
| } | |
| return nil | |
| } | |
| if prop != nil { | |
| mu.Lock() | |
| for key, value := range prop.Details { | |
| detail.Details[target+"-"+key] = value | |
| } | |
| mu.Unlock() | |
| if prop != nil { | |
| if prop.Details != nil { | |
| mu.Lock() | |
| for key, value := range prop.Details { | |
| detail.Details[target+"-"+key] = value | |
| } | |
| mu.Unlock() | |
| } | |
| } |
Signed-off-by: Kosuke Morimoto <[email protected]>
Signed-off-by: Kosuke Morimoto <[email protected]>
kmrmt
deleted the
backport/release/v1.7/feature/agent/ngt-property-api
branch
Description
Add API for getting NGT Property
Related Issue
Versions
Checklist
Special notes for your reviewer
Summary by CodeRabbit
New Features
Enhancements
Documentation
Version Update
1.22.6to1.23.0.