-
Notifications
You must be signed in to change notification settings - Fork 27
chore: add more tests + small improvements for .NET wrapper #689
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Adds some more tests for the .NET wrapper and fixes some issues with multi-statement SQL strings that return errors for the second (or later) statement. This change also removes the option to use unary RPCs to fetch results using the gRPC API in the .NET wrapper, as the performance of that is slower than using a streaming RPC. This will in a future pull request be replaced with a protocol that uses a bi-directional gRPC stream.
Summary of ChangesHello @olavloite, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly improves the robustness and test coverage of the .NET wrapper for SpannerLib. Key changes include fixing issues related to multi-statement SQL execution, optimizing result fetching by exclusively using streaming gRPC RPCs, and adding comprehensive tests for various scenarios, including partial result consumption and error handling within multi-statement queries. These updates ensure a more reliable and performant client experience. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request refactors the ExecuteStreaming logic in spannerlib/grpc-server/server.go into a new streamRows helper function, improving error handling for NextResultSet and moving the CloseRows defer statement. In the C# wrapper (spannerlib-dotnet-grpc-impl/GrpcLibSpanner.cs), the GrpcLibSpanner class is made sealed, client-side load balancing is introduced by managing multiple gRPC channels and clients, and all API calls are updated to use a randomly selected client. The _useStreamingRows flag is removed, making streaming the default execution path, and helper methods are made private. The StreamingRows.cs file receives a license header, and its NextResultSet methods are updated to pre-fetch the next row and clear metadata. The mock server (spannerlib-dotnet-mockserver/MockSpannerServer.cs) gains new helper methods for creating specific result sets, and RandomResultSetGenerator.cs is enhanced to optionally generate null values and correctly format Int64 values as strings. Test files (BasicTests.cs, RowsTests.cs) are updated to enable previously ignored async tests, parameterize existing tests, and add new tests for handling multiple queries, stopping row consumption midway, and error scenarios during mixed statement execution. Review comments highlight that the use of Random.Shared in RandomResultSetGenerator.cs and RowsTests.cs for generating test data and scenarios makes tests non-deterministic, recommending refactoring for deterministic and reliable test execution.
spannerlib/wrappers/spannerlib-dotnet/spannerlib-dotnet-tests/RowsTests.cs
Show resolved
Hide resolved
spannerlib/wrappers/spannerlib-dotnet/spannerlib-dotnet-tests/RowsTests.cs
Show resolved
Hide resolved
| return s.streamRows(queryContext, rows, stream) | ||
| } | ||
|
|
||
| func (s *spannerLibServer) streamRows(queryContext context.Context, rows *pb.Rows, stream grpc.ServerStreamingServer[pb.RowData]) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moving this logic into a separate function makes it easier to re-use. It will be re-used in the bi-directional gRPC stream implementation in a follow-up PR.
| if err := stream.Send(res); err != nil { | ||
| return err | ||
| } | ||
| if nextResultSetErr != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This ensures that calling NextResultSet when using the streaming gRPC API returns an error if the second (or third, ...) SQL statement in a multi-statement SQL string fails.
| private readonly V1.SpannerLib.SpannerLibClient _client; | ||
| private readonly GrpcChannel _channel; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replaced this with an implementation that always uses a (very simple) channel pool.
| private readonly GrpcChannel _channel; | ||
| private readonly V1.SpannerLib.SpannerLibClient[] _clients; | ||
| private readonly GrpcChannel[] _channels; | ||
| private readonly bool _useStreamingRows; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed this option, as there is no good reason why we would not use the streaming implementation.
| public Pool CreatePool(string connectionString) | ||
| { | ||
| return FromProto(TranslateException(() => _client.CreatePool(new CreatePoolRequest | ||
| return FromProto(TranslateException(() => Client.CreatePool(new CreatePoolRequest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This picks a channel from the 'pool' instead of always just using one fixed channel.
|
|
||
| return HasNextResultSet(); | ||
| var hasNextResultSet = HasNextResultSet(); | ||
| _pendingRow = Next(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This call is needed to ensure that the error is propagated to the application if the second (or third, ...) statement in a multi-statement SQL string failed.
bhatt4982
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM...
Adds some more tests for the .NET wrapper and fixes some issues with multi-statement SQL strings that return errors for the second (or later) statement.
This change also removes the option to use unary RPCs to fetch results using the gRPC API in the .NET wrapper, as the performance of that is slower than using a streaming RPC. This will in a future pull request be replaced with a protocol that uses a bi-directional gRPC stream.