Repository navigation
fix(spanner,bigtable): preserve streaming RPC error status on retry timeout and incomplete rows - #16616
fix(spanner,bigtable): preserve streaming RPC error status on retry timeout and incomplete rows#16616andrewro-google wants to merge 1 commit into
Conversation
andrewro-google
changed the title
![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 refactors the retry loops in both Bigtable and Spanner's PartialResultSetResume to check if the retry policy is exhausted before recreating the reader, addressing issues where the retry deadline expires during backoff. It also updates PartialResultSetSource to correctly surface RPC errors when a stream ends with uncommitted or incomplete rows, and adds comprehensive unit tests. The reviewer suggested using break instead of returning false directly from the loop in Spanner's PartialResultSetResume to align with the Bigtable implementation and avoid potential compiler warnings.
| if (retry_policy_prototype_->IsExhausted()) return false; | ||
| resumption = true; | ||
| last_status_.reset(); | ||
| child_ = factory_(*resume_token); | ||
| } while (!retry_policy_prototype_->IsExhausted()); | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
To improve consistency with the Bigtable implementation and prevent potential compiler warnings (such as 'control reaches end of non-void function') or future maintenance issues if a break is ever introduced inside the loop, consider using break; instead of returning false directly from inside the loop, and keep the return false; statement at the end of the function.
if (retry_policy_prototype_->IsExhausted()) break;
resumption = true;
last_status_.reset();
child_ = factory_(*resume_token);
}
return false;
}There was a problem hiding this comment.
I disagree. The consistency between bigtable and spanner isn't important. I fixed both because the bug is in both. If a break is ever introduced the code won't compile since there's no return statement after the loop.
6c98c88 to
a7c7e74
Compare
|
@scotthart PTAL when you have a chance — thanks! |
|
/gcbrun |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16616 +/- ##
==========================================
+ Coverage 92.51% 92.53% +0.01%
==========================================
Files 2269 2269
Lines 222804 223107 +303
==========================================
+ Hits 206126 206450 +324
+ Misses 16678 16657 -21 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
/gcbrun |
…ete rows
In `PartialResultSetResume::Read` (spanner and bigtable), when a retryable
streaming error occurred and the retry policy's deadline expired during backoff
(`sleep_for`), the loop reset `last_status_` and created a new unread
`PartialResultSetReader` before checking `IsExhausted()`. Subsequent calls to
`Finish()` returned `OK` from the unread reader, causing
`PartialResultSetSource::Create` to return a misleading `INTERNAL:
PartialResultSetSource response contained no metadata` error before metadata
arrived, or silently truncating results mid-stream. Check `IsExhausted()`
immediately after backoff before resetting `last_status_` or creating a new
reader.
In `PartialResultSetSource::ReadFromStream` (spanner and bigtable), when a
stream ended with buffered incomplete row or uncommitted row data, check
`reader_->Finish()` first and surface any non-OK RPC `Status` rather than
masking it with `INTERNAL` ("PartialResultSetSource stream ended at a point that
is not on a row boundary" / "Stream ended with uncommitted rows.").
4694c53 to
ffe1853
Compare
|
/gcbrun |
In
PartialResultSetResume::Read(spanner and bigtable), when a retryable streaming error occurred and the retry policy's deadline expired during backoff (sleep_for), the loop resetlast_status_and created a new unreadPartialResultSetReaderbefore checkingIsExhausted(). Subsequent calls toFinish()returnedOKfrom the unread reader, causingPartialResultSetSource::Createto return a misleadingINTERNAL: PartialResultSetSource response contained no metadataerror before metadata arrived, or silently truncating results mid-stream. CheckIsExhausted()immediately after backoff before resettinglast_status_or creating a new reader.In
PartialResultSetSource::ReadFromStream(spanner and bigtable), when a stream ended with buffered incomplete row or uncommitted row data, checkreader_->Finish()first and surface any non-OK RPCStatusrather than masking it withINTERNAL("PartialResultSetSource stream ended at a point that is not on a row boundary" / "Stream ended with uncommitted rows.").Includes deterministic zero-sleep unit tests for both libraries covering retry deadline expiration during backoff and streaming RPC errors on incomplete rows.