impl(storage): add appendable upload builder and writer#5697
impl(storage): add appendable upload builder and writer#5697joshuatants wants to merge 2 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 initial framework for appendable object uploads, adding the OpenAppendableObject builder and AppendableObjectWriter under the google_cloud_unstable_storage_bidi feature flag. Review feedback highlighted that the builder lacks configuration methods for request parameters, which is inconsistent with other SDK builders. Furthermore, the use of unimplemented!() in the library code was identified as a violation of the style guide's prohibition on panics.
| unimplemented!() | ||
| } | ||
|
|
||
| /// Flushes any pending data to the server and awaits durable persistence on | ||
| /// the server. | ||
| pub async fn flush(&mut self) -> Result<()> { | ||
| unimplemented!() | ||
| } | ||
|
|
||
| /// Finalizes the upload, sending any pending data. Returns the final [Object][crate::model::Object]. | ||
| pub async fn finalize(self) -> Result<Object> { | ||
| unimplemented!() | ||
| } | ||
|
|
||
| /// Relinquishes the writer without finalizing, draining any pending data to the server. | ||
| /// Returns the final persisted size of the object. | ||
| pub async fn close(self) -> Result<i64> { | ||
| unimplemented!() | ||
| } | ||
|
|
||
| /// Returns the latest durable offset confirmed by the server. | ||
| pub fn persisted_size(&self) -> i64 { | ||
| unimplemented!() | ||
| } | ||
|
|
||
| /// Returns the server-issued write handle, if available. | ||
| pub fn write_handle(&self) -> Option<BidiWriteHandle> { | ||
| unimplemented!() | ||
| } | ||
|
|
||
| /// Returns the generation of the object being appended to. | ||
| pub fn generation(&self) -> i64 { | ||
| unimplemented!() | ||
| } | ||
|
|
||
| /// Returns the server-issued routing token, if available. | ||
| pub fn routing_token(&self) -> Option<String> { | ||
| unimplemented!() | ||
| } | ||
|
|
||
| /// Returns the latest known object metadata. | ||
| pub fn object(&self) -> Option<Object> { | ||
| unimplemented!() |
There was a problem hiding this comment.
While features can be partially implemented if documented, please avoid using unimplemented!() in library code as it causes panics. Even for unstable features, it is preferred to return a Result with an error or a default value to ensure the application remains stable during development.
References
- No panic! macro calls in library code. (link)
- A feature does not have to be fully implemented in a single pull request if the remaining work is documented in the PR description or as a TODO comment in the code.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #5697 +/- ##
==========================================
- Coverage 97.89% 97.80% -0.09%
==========================================
Files 226 228 +2
Lines 55536 55587 +51
==========================================
+ Hits 54366 54368 +2
- Misses 1170 1219 +49 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
joshuatants
force-pushed
the
appendable_uploads_skeleton
branch
3 times, most recently
from
444768e to
d000a0e
Compare
|
LGTM. |
| /// writer.append(Bytes::from_static(b"hello ")).await?; | ||
| /// writer.append(Bytes::from_static(b"world")).await?; | ||
| /// writer.flush().await?; | ||
| /// | ||
| /// let object = writer.finalize().await?; |
There was a problem hiding this comment.
As you know, this inverts the control from the existing writes (and yes, Rust is inverted from the other SDKs).
You could consider:
client.append_to_object("bucket", "object", stream).send().await?where stream is an asynchronous stream of
enum {
// Add some data
Data(Bytes),
// Relinquish maybe with some extra bytes.
Close(Option<Data>),
// Ditto but finalize
Finalize(Option<Data>),
}Dunno, may be overly complicated.
There was a problem hiding this comment.
Thanks for the pointer. We'll have a think about it. For now, can we get this merged so that we can continue working on the internal plumbing? We should be able to modify this later as long as it's done before we release the API.
joshuatants
force-pushed
the
appendable_uploads_skeleton
branch
from
adfa1a7 to
8e1d1af
Compare
joshuatants
force-pushed
the
appendable_uploads_skeleton
branch
from
8e1d1af to
4ae04db
Compare
joshuatants
changed the title
| use crate::model::Object; | ||
| use bytes::Bytes; | ||
|
|
||
| /// A handle to an in-progress appendable object upload. |
There was a problem hiding this comment.
Maybe you want to add a TODO(#5716) at the top of the file?
3 participants
No description provided.