impl(bigquery): generate job/request IDs and mark query RPCs idempotent - #6219
impl(bigquery): generate job/request IDs and mark query RPCs idempotent#6219alvarowolfx 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 introduces unique ID generation for BigQuery jobs and query requests using UUIDs to enable request idempotency. It updates the execution paths to set idempotency options and adds corresponding unit tests. The review feedback suggests refactoring generate_job_reference into an associated function to avoid borrowing &self and eliminate an unnecessary clone of project_id, which also simplifies its unit test.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6219 +/- ##
=======================================
Coverage 96.02% 96.02%
=======================================
Files 269 269
Lines 67282 67330 +48
=======================================
+ Hits 64606 64654 +48
Misses 2676 2676 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| mock.expect_query().returning(move |req, _| { | ||
| let req_id = &req.query_request.as_ref().unwrap().request_id; | ||
| assert!(req_id.starts_with(QUERY_REQUEST_ID_PREFIX), "{req_id:?}"); | ||
| assert!(req_id.len() <= 36, "{req_id:?}"); // bigquery limits request id to 36 characters |
There was a problem hiding this comment.
I would prefer this as a const, but since it is unused maybe thats fine. Maybe const _BIGQUERY_REQ_ID_LIMIT = 36 or something in the code?
| Job, JobConfiguration, JobReference, JobStatus, QueryRequest, QueryResponse, | ||
| }; | ||
| use google_cloud_gax::response::Response; | ||
| use uuid::Uuid; |
There was a problem hiding this comment.
I think this might be unnecessary with use super::*;
| if !location.is_empty() { | ||
| job_ref = job_ref.set_location(location.to_string()); | ||
| } | ||
|
|
There was a problem hiding this comment.
nit(style): consider removing the new line before the implicit return.
| job_ref | ||
| } | ||
|
|
||
| fn generate_prefixed_id(prefix: &str) -> String { |
There was a problem hiding this comment.
Since the size of the ID matters for each of the places we use this (to varying degrees), it would be nice to have a comment explaining why this is in range for both of them / maybe defining what the returned size of the id will be as a guarantee of this function.
| } | ||
|
|
||
| fn generate_job_reference(project_id: &str, location: &str) -> JobReference { | ||
| let job_id = generate_prefixed_id(JOB_ID_PREFIX); |
There was a problem hiding this comment.
nit: could be nice to have a link to any specs for what this ID should look like.
| .execute() | ||
| .await | ||
| } else { | ||
| let query_request_id = generate_prefixed_id(QUERY_REQUEST_ID_PREFIX); |
There was a problem hiding this comment.
nit: could be nice to have a link to any specs for what this ID should look like (including that 36 limit mentioned below)
supernit: for symmetry across if-else block, consider moving this initialization to let query_request = generate_query_request(...);
| .insert_job() | ||
| .with_request(self.request) | ||
| // jobs.insert is idempotent because every request | ||
| // carries a generated UUID job_id. |
There was a problem hiding this comment.
supernit: remove UUID (or add it above), since you also add the prefix it is not a pure UUID
| mock.expect_insert_job().returning(|_, _| { | ||
| mock.expect_insert_job().returning(|req, _| { | ||
| let job_ref = req.job.as_ref().unwrap().job_reference.as_ref().unwrap(); | ||
| assert!(job_ref.job_id.starts_with(JOB_ID_PREFIX), "{job_ref:?}"); |
There was a problem hiding this comment.
nit: This technically has a length limit too (but its very large). But could still be worth checking explicitly? Your call.
2 participants
Towards #5844 #6218