fix(spanner-ado-net): prevent sync calls on async execution paths for DML stats#776
fix(spanner-ado-net): prevent sync calls on async execution paths for DML stats#776olavloite wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces asynchronous support for retrieving Spanner query and DML execution statistics, adding StatsAsync to the ISpannerLib interface and implementing GetStatsAsync and GetUpdateCountAsync in the Rows class. The review feedback highlights several critical issues: GetTotalUpdateCountAsync can miss update counts for DML with RETURNING clauses if rows are not consumed first; adding StatsAsync to ISpannerLib without a default implementation breaks backward compatibility; and redundant remote calls may occur when stats are null, which can be resolved by introducing and properly resetting a _statsLoaded boolean flag.
84ec46c to
4234295
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces asynchronous methods for retrieving result set statistics and update counts (such as GetStatsAsync and GetUpdateCountAsync) in the Spanner ADO.NET library, along with corresponding unit tests. Feedback highlights a potential sync-over-async leak in GetStatsAsync where _stats.IsValueCreated remains false after loading stats asynchronously, which could trigger redundant synchronous IPC calls during subsequent synchronous operations. Re-initializing _stats with a lazy wrapper of the loaded stats is recommended to resolve this.
4234295 to
b38e2c5
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces asynchronous support for retrieving query statistics and update counts in the Spanner ADO.NET provider. It adds StatsAsync to ISpannerLib and implements GetStatsAsync and GetUpdateCountAsync in Rows to prevent synchronous blocking (sync-over-async) when retrieving DML update counts. Comprehensive unit tests are also added to verify these asynchronous paths and cache clearing behaviors. The reviewer suggested optimizing the GetStatsAsync implementation in Rows.cs by modifying the Next() method to check _statsLoaded instead of re-initializing the _stats Lazy instance, which avoids unnecessary allocations.
… DML stats Introduced asynchronous StatsAsync methods in ISpannerLib and GrpcLibSpanner, along with async GetStatsAsync and GetUpdateCountAsync helpers in Rows.cs. This enables GetTotalUpdateCountAsync to run completely asynchronously. In StreamingRows, GetStatsAsync returns the local cached stats in memory, ensuring no remote blocking IPC calls are executed during asynchronous DML execution.
b38e2c5 to
4fc90c3
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces asynchronous support for retrieving ResultSetStats from a Rows object, preventing synchronous-over-asynchronous blocking calls. Key changes include adding StatsAsync to the ISpannerLib interface and its gRPC implementation, introducing GetStatsAsync and GetUpdateCountAsync in Rows.cs to fetch and cache stats asynchronously, and updating GetTotalUpdateCountAsync to utilize these asynchronous paths. Additionally, new unit tests were added in CommandTests.cs and RowsTests.cs to validate the asynchronous behavior and cache-clearing logic. I have no feedback to provide as there are no review comments.
Introduced asynchronous StatsAsync methods in ISpannerLib and GrpcLibSpanner, along with async GetStatsAsync and GetUpdateCountAsync helpers in Rows.cs.
This enables GetTotalUpdateCountAsync to run completely asynchronously. In StreamingRows, GetStatsAsync returns the local cached stats in memory, ensuring no remote blocking IPC calls are executed during asynchronous DML execution.