.NET SDK rework #25
Merged
.NET SDK rework #25
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
- It shouldn't create a new HttpClient instance for each MilvusRestClient. If one isn't supplied to the ctor, it should use a shared instance. - It shouldn't configure a supplied HttpClient instance with a base Uri or default headers. Doing so would mutate an HttpClient potentially being used for other purposes. - It shouldn't Dispose of a supplied HttpClient instance, as ownership isn't being transferred. - Also marked the client as sealed so that it needn't implement the IDisposable pattern and deal with possible derived types. And then related removed the duplicative Close that's unnecessary with Dispose existing. - Removed duplicative BaseAddress.
- Seal it, and then avoid implementing the IDisposable pattern - Avoid disposing of a GrpcChannel passed in, as this client does not have ownership of it
- Dedup methods - Avoid serializing to an intermediate throwaway string - Avoid a new HttpMethod for PATCH on every call
The IGrpcRequest, IRestRequest, and IValidatable interfaces don't seem to be serving a purpose. They're implemented but never consumed.
…arp into feat-test_in_ci
The files under ApiSchema were attempting to centralize argument handling across the gRPC and REST clients. However, this led to complications around things like argument validation, such as arguments not being validated before they were used, and also made it harder to see the flow of data from arguments into the REST / gRPC calls. We're also contemplating whether we need the REST client at all, at which point such centralization only adds complexity.
Makes it easier to reason about the surface area and version. We can selectively unseal as needed.
remove MilvusVectors.cs
* fixed oldpassword enocder error * fixed test error * Apply suggestions from code review Co-authored-by: Shay Rojansky <[email protected]> * add missing namespace. * simplify code --------- Co-authored-by: Shay Rojansky <[email protected]>
* remove extra configuration for Zilliz Cloud * remove extra config for zilliz cloud
* Enforce some analyzer rules * Make errors into warnings
Add flush methods and tests
Notably make QueryAsync accept a parameter object rather than many optional parameters, aligning it to SearchAsync.
weianweigan
approved these changes
Aug 6, 2023
|
Thanks! It's great that we will be merging into the milvus repository. Let's merge it now? |
|
It's ready from merging as far as I'm concerned (I don't have write access to do that though). |
Ok, Let me merge it now. (●'◡'●) |
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.
/cc @weianweigan @yhmo @stephentoub @tawalke @luisquintanilla @LadyNaggaga @ajcvickers