feat(storage): implement appendable upload background worker#6001
feat(storage): implement appendable upload background worker#6001vsharonlynn wants to merge 3 commits into
Conversation
product-auto-label
Bot
added
the
api: storage
![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 the internal infrastructure for Appendable Object Write (Bidi Write) in Google Cloud Storage, including a Connector to handle the initial handshake and redirects, and a background Worker to manage the live gRPC stream and upload intents. The feedback highlights two key issues: first, preconditions (if_metageneration_match and if_metageneration_not_match) are lost when transitioning from WriteObjectSpec to AppendObjectSpec in the connector; second, the worker exits silently with Ok(()) if the server closes the stream unexpectedly while the client is idle, which should instead be treated as an error.
| AppendObjectSpecState::Write(s, _) => { | ||
| AppendObjectSpecState::Append(AppendObjectSpec { | ||
| bucket: s | ||
| .resource | ||
| .as_ref() | ||
| .map(|r| r.bucket.clone()) | ||
| .unwrap_or_default(), | ||
| object: s | ||
| .resource | ||
| .as_ref() | ||
| .map(|r| r.name.clone()) | ||
| .unwrap_or_default(), | ||
| generation: new_generation.unwrap_or(0), | ||
| write_handle: m.write_handle.clone(), | ||
| ..Default::default() | ||
| }) | ||
| } |
There was a problem hiding this comment.
Preconditions (if_metageneration_match and if_metageneration_not_match) are lost when transitioning from WriteObjectSpec to AppendObjectSpec upon successful connection. This can cause preconditions to be silently ignored if a reconnect happens later. Copy these fields from s to the new AppendObjectSpec to preserve them.
AppendObjectSpecState::Write(s, _) => {
AppendObjectSpecState::Append(AppendObjectSpec {
bucket: s
.resource
.as_ref()
.map(|r| r.bucket.clone())
.unwrap_or_default(),
object: s
.resource
.as_ref()
.map(|r| r.name.clone())
.unwrap_or_default(),
generation: new_generation.unwrap_or(0),
write_handle: m.write_handle.clone(),
if_metageneration_match: s.if_metageneration_match,
if_metageneration_not_match: s.if_metageneration_not_match,
..Default::default()
})
}| Ok(None) => { | ||
| if !self.pending_flushes.is_empty() { | ||
| return Some(Err(Arc::new(crate::Error::io( | ||
| "stream closed unexpectedly", | ||
| )))); | ||
| } | ||
| return None; | ||
| } |
There was a problem hiding this comment.
If the server closes the stream unexpectedly while the client is idle (i.e., pending_flushes is empty), handle_response returns None, which causes the worker to exit silently with Ok(()). This can make subsequent client writes fail with a generic channel closed error instead of a descriptive stream closed error.
Consider tracking whether the upload has been finalized (e.g., via a completed flag set when Finalize is processed), and treat any stream close (Ok(None)) before finalization as an error.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6001 +/- ##
========================================
Coverage 97.73% 97.73%
========================================
Files 244 247 +3
Lines 61096 61832 +736
========================================
+ Hits 59710 60434 +724
- Misses 1386 1398 +12 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1 participant
This PR comes after #5956.