fix(bigquery): handle 409 errors and attach to existing job - #6780
alvarowolfx wants to merge 6 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces robust handling for duplicate job errors (409 Already Exists) during BigQuery query executions, allowing the client to adopt an existing job from a previous attempt instead of failing. It also refactors retry policies to ensure non-pre-RPC errors are only retried for idempotent requests, and adds comprehensive unit tests. Feedback is provided to optimize the check_job_status helper by using std::mem::take to avoid an unnecessary clone of the error list, adhering to the repository's performance guidelines.
| fn check_job_status(job: Job) -> Result<Job> { | ||
| if let Some(status) = job.status.as_ref() | ||
| && status.error_result.is_some() | ||
| { | ||
| let errors = status.errors.clone(); | ||
| return Err(QueryError::JobFailed { errors }); | ||
| } | ||
|
|
||
| Ok(job) | ||
| } |
There was a problem hiding this comment.
Since job is consumed by check_job_status, we can avoid cloning status.errors by taking ownership of the errors using std::mem::take on a mutable reference to the status. This avoids unnecessary allocations and clones, adhering to the repository style guide's guidance on avoiding unnecessary clones.
| fn check_job_status(job: Job) -> Result<Job> { | |
| if let Some(status) = job.status.as_ref() | |
| && status.error_result.is_some() | |
| { | |
| let errors = status.errors.clone(); | |
| return Err(QueryError::JobFailed { errors }); | |
| } | |
| Ok(job) | |
| } | |
| fn check_job_status(mut job: Job) -> Result<Job> { | |
| if let Some(status) = job.status.as_mut() | |
| && status.error_result.is_some() | |
| { | |
| let errors = std::mem::take(&mut status.errors); | |
| return Err(QueryError::JobFailed { errors }); | |
| } | |
| Ok(job) | |
| } |
References
- Scrutinize expensive uses of clone() (i.e. for Strings, not for Arcs). Is it necessary to copy the data? Can we move the data instead? (link)
|
might need to split since handling this kind of error is tricky.
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6780 +/- ##
==========================================
- Coverage 97.03% 97.02% -0.01%
==========================================
Files 326 326
Lines 107511 107706 +195
==========================================
+ Hits 104322 104507 +185
- Misses 3189 3199 +10 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Towards #6717