Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a new example, OptimizeWriteLatencyPool, to demonstrate optimizing write latency using a pool of pre-warmed appendable object uploads. The review feedback identifies a critical lifetime issue where the asynchronous lambda maintain_pool captures local variables by reference, which could lead to undefined behavior if the outer coroutine exits early. It is recommended to refactor this lambda to pass parameters by value and return the new writer and token. Additionally, the use of auto for the client.Open return type should be replaced with explicit typing to adhere to the repository's style guide against obscured domain types.
d06c54c to
cb43369
Compare
cb43369 to
991d229
Compare
Adds OptimizeWriteLatencyPool sample (region tag: storage_optimize_write_latency_pool) to storage_async_samples.cc demonstrating a pre-warmed pool of AsyncWriter instances with unfinalized objects and Flush() to avoid object creation and finalization metadata overhead on the critical write path. Verified with both mock unit tests and live integration testing against a Rapid (zonal) bucket in us-central1-a: Running live C++ test against bucket=<zonal-bucket>, prefix=live_cpp_pool_1790087459 C++ read back: "0123456789", pool size after refill: 3 [ OK ] WriterPoolCppTest.PreWarmedPoolWithUnfinalizedObjects (1464 ms) [ PASSED ] 1 test.
991d229 to
afc7261
Compare
|
/gcbrun |
|
/gcbrun |
|
/gcbrun |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #16487 +/- ##
==========================================
- Coverage 92.37% 92.36% -0.01%
==========================================
Files 2262 2262
Lines 217100 217100
==========================================
- Hits 200540 200526 -14
- Misses 16560 16574 +14 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
kalragauri
left a comment
There was a problem hiding this comment.
nit: The second and third bullets in the PR description still mention calling Flush() during pool initialization and the (~1-2 ms) latency figure. In C++, AsyncClient::StartAppendableObjectUpload eagerly sends the initial BidiWriteObjectRequest (state_lookup = true) and awaits the server's initial response before resolving, so the final code no longer calls Flush() during pool initialization (and commit 4a7e02e removed the ~1-2 ms figure).
Pls update these two bullets in the PR description before merging.
| PauseAndResumeAppendableUpload), | ||
| make_entry("finalize-appendable-object-upload", {}, | ||
| FinalizeAppendableObjectUpload), | ||
| make_entry("optimize-write-latency-pool", {}, OptimizeWriteLatencyPool), |
There was a problem hiding this comment.
nit: make_entry hardcodes in the --help output, whereas this sample takes a <key-prefix> to create multiple objects.
For consistency with how custom CLI arguments are registered in this file, consider using make_bucket_entry("optimize-write-latency-pool", {"<key-prefix>"}, OptimizeWriteLatencyPool).
| -> google::cloud::future<std::pair<gcs::AsyncWriter, gcs::AsyncToken>> { | ||
| auto close_status = co_await writer.Close(); | ||
| if (!close_status.ok()) throw std::runtime_error(close_status.message()); | ||
| auto [new_writer, new_token] = |
There was a problem hiding this comment.
nit: since bucket_name and next_object_name are passed by value into maintain_pool and not used again, moving them avoids an extra copy. Consider passing gcs::BucketName(std::move(bucket_name)), std::move(next_object_name) instead.
Adds
OptimizeWriteLatencyPoolsample (region tag:storage_optimize_write_latency_pool) tostorage_async_samples.ccdemonstrating a pre-warmed pool ofAsyncWriterinstances with unfinalized objects andFlush()to avoid object creation and finalization metadata overhead on the critical write path.Key features:
std::deque) ofAsyncWriterobjects on appendable objects.Flush()on the 0-byte objects during pool initialization to force the lazy gRPC metadata round-trip ahead of time.Flush()(~1-2 ms) instead ofFinalize().Close()the used writer without finalizing and refill the pool."optimize-write-latency-pool"and automated testing inAutoRun().Verified with both mock unit tests and live integration testing against a Rapid (zonal) bucket in
us-central1-a: