chore: add bidirectional streaming API to SpannerLib #690
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.
|
/gemini review |
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 enhances SpannerLib by introducing a bidirectional gRPC streaming API. This new approach aims to reduce latency and improve efficiency for clients, particularly the .NET wrapper, by allowing multiple operations to be multiplexed over a single connection. The changes involve extensive updates to protobuf definitions, server-side Go implementations, and the .NET client library to fully leverage this new communication paradigm. 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for a bidirectional streaming API to SpannerLib, along with updates to the .NET wrapper and associated tests. The changes include new gRPC service methods, message definitions, and client-side implementations to leverage the new streaming capabilities. While the core functionality seems well-implemented, there are a few areas that could be improved for robustness, clarity, and adherence to best practices.
I am having trouble creating individual review comments. Click here to see my feedback.
spannerlib/grpc-server/google/spannerlib/v1/spannerlib.proto (77-78)
The num_rows field is marked as REQUIRED, but the C# client (e.g., GrpcBidiConnection.cs line 176) sends prefetchRows which can be 0. The Go server also defaults maxFetchRows to 50 if NumRows is 0. This indicates that 0 is a valid value and the field should not be REQUIRED. Marking a field as REQUIRED when 0 is acceptable can lead to validation errors or unexpected behavior if strict proto validation is applied.
int64 num_rows = 1;
spannerlib/grpc-server/server_test.go (251-350)
The TestExecuteStreamingMultiQuery test case was removed. This test covered scenarios with multiple queries in a single streaming execution. Given the introduction of a new bidirectional streaming API, it's crucial to ensure that multi-query scenarios are still adequately tested, especially with the new API. Removing this test without a clear replacement for the new API might leave a gap in test coverage.
spannerlib/grpc-server/google/spannerlib/v1/spannerlib.proto (150-151)
The fetch_options field in NextRequest is marked as REQUIRED. However, the FetchOptions message itself has fields (num_rows, encoding) that are used with default/zero values in the client, which contradicts the REQUIRED behavior. This inconsistency can lead to validation errors or unexpected behavior.
FetchOptions fetch_options = 2;
spannerlib/grpc-server/google/spannerlib/v1/spannerlib.proto (80-81)
The encoding field is marked as REQUIRED. However, its usage in the C# client and Go server does not strictly enforce a non-zero value. If 0 is a valid default or acceptable value, this field should not be REQUIRED.
int64 encoding = 2;
spannerlib/api/rows.go (95)
Using /*marshalResult=*/ as a comment for a function argument is not idiomatic Go. Go does not support named parameters in function calls in this way. This comment can be confusing and should be removed.
values, _, err := next(ctx, poolId, connId, rowsId, false, resetBuffer)
spannerlib/api/rows.go (73)
Using /*marshalResult=*/ and /*resetBuffer=*/ as comments for function arguments is not idiomatic Go. Go does not support named parameters in function calls in this way. These comments can be confusing and should be removed.
_, bytes, err := next(ctx, poolId, connId, rowsId, true, false)
spannerlib/api/rows.go (91)
Using /*resetBuffer=*/ as a comment for a function argument is not idiomatic Go. Go does not support named parameters in function calls in this way. This comment can be confusing and should be removed.
return nextWithBufferOption(ctx, poolId, connId, rowsId, false)
spannerlib/grpc-server/server.go (230)
The value 50 is a magic number. It would be clearer and more maintainable to define this as a named constant.
const defaultMaxFetchRows = 50
maxFetchRows := int64(defaultMaxFetchRows)spannerlib/grpc-server/server.go (258-264)
The error handling logic here is a bit complex. If an error occurs while fetching rows, and it's the first result set, the function returns nil, err. If it's a subsequent result set, it removes the last result set from the response and sets an error status. This distinction might be confusing. Consider simplifying to either always return the error directly or always return partial results with an error status, depending on the desired behavior for partial success.
spannerlib/grpc-server/server.go (279-285)
Similar to the previous comment, the error handling logic here is complex. If an error occurs while fetching stats, and it's the first result set, the function returns nil, err. If it's a subsequent result set, it removes the last result set from the response and sets an error status. This distinction might be confusing. Consider simplifying to either always return the error directly or always return partial results with an error status, depending on the desired behavior for partial success.
spannerlib/grpc-server/server_test.go (137-138)
The assertion for the actual string value of the row (row.Values[0].GetStringValue()) was removed. This reduces the thoroughness of the test, as it no longer verifies the content of the data, only its presence and count. It's important to verify the actual data to ensure correctness.
if g, w := row.Values[0].GetStringValue(), fmt.Sprintf("%d", numRows); g != w {
t.Fatalf("value mismatch\n Got: %v\nWant: %v", g, w)
}
spannerlib/api/rows.go (82)
Using /*resetBuffer=*/ as a comment for a function argument is not idiomatic Go. Go does not support named parameters in function calls in this way. This comment can be confusing and should be removed.
return nextWithBufferOption(ctx, poolId, connId, rowsId, true)
spannerlib/grpc-server/server_test.go (435-436)
The assertion for the actual string value of the row (row.Data[0].Values[0].GetStringValue()) was removed. This reduces the thoroughness of the test, as it no longer verifies the content of the data, only its presence and count. It's important to verify the actual data to ensure correctness.
if g, w := row.Data[0].Values[0].GetStringValue(), fmt.Sprintf("%d", numRows); g != w {
t.Fatalf("value mismatch\n Got: %v\nWant: %v", g, w)
}
There was a problem hiding this comment.
Code Review
This pull request introduces a bidirectional streaming API to SpannerLib and updates the .NET wrapper to support it. The changes are extensive and well-structured, particularly the protobuf definitions and the C# implementation which cleverly abstracts both the old and new streaming models. The new feature is also well-tested in the .NET wrapper.
My review focuses on the Go server implementation and its tests. I've identified a potential performance improvement in the new bidirectional stream handler. Additionally, I've noticed that some tests have been weakened by removing assertions, and one important test case for the existing server-streaming API has been removed entirely. Please see my detailed comments for suggestions.
I am having trouble creating individual review comments. Click here to see my feedback.
spannerlib/grpc-server/server_test.go (251-350)
This test for ExecuteStreaming with multiple queries has been removed. While new tests for multi-statement execution have been added for bidirectional streaming, this removal reduces test coverage for the existing server-streaming ExecuteStreaming RPC. Was this intentional? If the feature is still supported, it would be good to keep a test for it.
spannerlib/grpc-server/server.go (256)
For better performance, you could use api.NextBuffered here to reuse buffers. This would be consistent with streamRows which also uses the buffered version.
To do this safely, you'll need to clone the returned row before appending it to the resultSet.Rows slice on line 269, as NextBuffered reuses the same ListValue instance. You can use proto.Clone for this, which will require importing google.golang.org/protobuf/proto.
Example of the change on line 269:
resultSet.Rows = append(resultSet.Rows, proto.Clone(row).(*structpb.ListValue))row, err := api.NextBuffered(ctx, rows.Connection.Pool.Id, rows.Connection.Id, rows.Id)
spannerlib/grpc-server/server_test.go (137-139)
This assertion, which verifies the content of the returned row, was removed. While the test still checks the number of rows, re-adding a check for the row content would make the test more robust against regressions.
spannerlib/grpc-server/server_test.go (238-240)
This assertion verifying the row content has been removed. Please consider re-adding it to ensure the correctness of the data being returned and make the test more robust.
spannerlib/grpc-server/server_test.go (523-525)
This assertion verifying the row content has been removed. Please consider re-adding it to ensure the correctness of the data being returned and make the test more robust.
olavloite
force-pushed
the
spanner-lib-bidi-grpc
branch
3 times, most recently
from
562e253 to
a65ea4a
Compare
Adds support for using a bidirectional gRPC stream for sending requests and receiving responses from SpannerLib. Also adds support for this to the .NET wrapper and modifies all tests to run using both the previous server-streaming API and the new bidirectional streaming API. The latter is the default for the .NET wrapper, and support for the server-streaming API might be removed in a future version.
olavloite
force-pushed
the
spanner-lib-bidi-grpc
branch
from
a65ea4a to
058b06d
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a bidirectional streaming API to SpannerLib for improved performance and latency, along with corresponding updates to the .NET wrapper and tests. The changes are generally well-implemented, but I've identified a few areas for improvement, mainly related to code clarity and consistency. Specifically, I've pointed out some confusing inline comments and suggested refactoring some test code to reduce duplication. Addressing these points will enhance the maintainability and readability of the new features.
2 participants
Adds support for using a bidirectional gRPC stream for sending requests and receiving responses from SpannerLib.
Also adds support for this to the .NET wrapper and modifies all tests to run using both the previous server-streaming API and the new bidirectional streaming API. The latter is the default for the .NET wrapper, and support for the server-streaming API might be removed in a future version.