rsc: update read_job transaction hit path #1671
Conversation
rust/rsc/src/bin/rsc/read_job.rs
Outdated
| }) | ||
| .await; | ||
| // Step 6: TODO before recording a hit and returning, need verification that objects from transaction did not change in | ||
| // database, or convert a hit into a miss |
There was a problem hiding this comment.
I have questions on how best to accomplish this verification from chaning a hit to a miss. Do I just query (output_files, output_symlinks, output_dirs) again and verify they are the same with the database? Seems kind of wasteful (what happens when another update happens while I am doing the verification equivalence check?).
Can we instead potentially rely on job::Entity's created_at field (or create a new one called updated_at) where we just quickly check timestamps are same before returning?
rust/rsc/src/bin/rsc/read_job.rs
Outdated
| .filter(entity::blob::Column::Id.is_in(ids.to_vec())) | ||
| .all(db) | ||
| .await | ||
| .map_err(|e| format!("Failed to query blobs: {}", e))?; |
There was a problem hiding this comment.
Hmm it seems like we could have done .await?.into_iter().map(<blob_map code from the next line)
There was a problem hiding this comment.
Yep, Ill change that
rust/rsc/src/bin/rsc/read_job.rs
Outdated
| // Ensure we have all requested blobs | ||
| for &id in ids { | ||
| if !blob_map.contains_key(&id) { | ||
| return Err(format!("Unable to find blob {} by id", id)); |
There was a problem hiding this comment.
How does this error condition differ from the failed to query blobs check above?
rust/rsc/src/bin/rsc/read_job.rs
Outdated
| let mut resolved_map = HashMap::new(); | ||
| for res in results { | ||
| let (id, resolved_blob) = res?; | ||
| resolved_map.insert(id, resolved_blob); | ||
| } |
There was a problem hiding this comment.
Is there no simpler constructor for this?
We only to do this iteratively because we wanted to resolve the urls in parallel, rather than with a map on the original blob_map?
| }) | ||
| .await; | ||
|
|
||
| let hash_copy = hash_for_spawns.clone(); |
There was a problem hiding this comment.
Why isn't this just hash.clone()?
AbrarQuazi
force-pushed
the
update-transaction-read-job
branch
from
c33b2a7 to
f126081
Compare
| .map(|m| { | ||
| let blob_id = m.blob_id; | ||
| let resolved_blob = resolved_blob_map.get(&blob_id).cloned().ok_or_else(|| { | ||
| format!("Missing resolved blob for {}", blob_id) |
There was a problem hiding this comment.
Do we want to track the job that had a blob fetch failure? I can see that being useful debug info when something is failing.
| }; | ||
|
|
||
| let job_id = matching_job.id; | ||
| let hash_copy = hash_for_spawns.clone(); |
There was a problem hiding this comment.
Again it feels like we didn't need to create this separate hash_for_spawns variable and could have instead continued to clone hash
No description provided.