feat(bigquery): heap-box futures and builder in JobPoller to prevent stack overflow - #6237
feat(bigquery): heap-box futures and builder in JobPoller to prevent stack overflow#6237haphungw wants to merge 2 commits into
Conversation
product-auto-label
Bot
added
the
api: bigquery
![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 updates JobPoller in BigQuery and LRO discovery to box the InsertJob builder and return pinned boxed futures from until_done. A critical issue was identified in operation.rs where assigning to *builder after moving builder will cause a compilation error; it is recommended to reassign a new Box to builder instead.
| && attempts < self.policy.job_level_attempt_limit | ||
| { | ||
| let retry_job = prepare_job_for_retry(job_result); | ||
| *builder = builder.set_job(retry_job); |
There was a problem hiding this comment.
This assignment will likely cause a compilation error because calling builder.set_job(...) moves the InsertJob out of the Box (moving builder), making it impossible to assign back to *builder. Instead, you should reassign a new Box to builder.
| *builder = builder.set_job(retry_job); | |
| builder = Box::new(builder.set_job(retry_job)); |
| && attempts < self.policy.job_level_attempt_limit | ||
| pub fn until_done( | ||
| self, | ||
| ) -> std::pin::Pin<Box<dyn std::future::Future<Output = Result<Job, JobPollerError>> + Send>> |
There was a problem hiding this comment.
why does the signature has to change that much ? isn't this gonna diverge from other LROs that are async and return just a Result ?
Maybe I don't understand Pin/Box enough to understand what is going on here.
There was a problem hiding this comment.
I was struggling to get this test pass due to stack overflow
https://github.com/googleapis/google-cloud-rust/pull/6232/changes#diff-824e62dc84d6a981c4c62f1f254e61712feacc64d410a3a1b0744f981cb657edR267
https://pantheon.corp.google.com/cloud-build/builds;region=us-central1/6916363e-91b1-4147-81c4-2bf87a03abd7?project=rust-sdk-testing&e=SqlStudioMysqlLaunch::SqlStudioMysqlEnabled
The builder and polling loop state machine get pretty huge on the stack, so boxing it offloads that to the heap.
The the .until_done().await caller syntax is still the same, but it seems like I haven't been able to fix the issue.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6237 +/- ##
==========================================
- Coverage 96.05% 96.04% -0.01%
==========================================
Files 269 269
Lines 68390 68390
==========================================
- Hits 65689 65685 -4
- Misses 2701 2705 +4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
2 participants
Heap-box
JobPoller::until_donefuture,JobPollerbuilder, andDiscoveryPoller::until_done.Job structs are large (~7.1 KB) and cause deep unboxed async future state machines to overflow the default 2 MB thread stack during integration tests.