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?
There was a problem hiding this comment.
So the error above checks for errors like database connection issues, and other database related errors
The error check here checks if all of the rows are returned from the query (there could be the scenario that a requested ID did not exist, so it didn't return anything for that).
Perhaps I can change the Error text for the condition above to be more like `Failed to query blobs, database error: {}" , e"
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?
There was a problem hiding this comment.
So I think I can do this instead of the manual for loop:
let resolved_map: HashMap<Uuid, ResolvedBlob> = results.into_iter().collect::<Result<_,_>>()?;
Yep thats exactly why we do it, we could technically add to the hashmap parallely too but I dont think there is any benefits and adds to complexity
| }) | ||
| .await; | ||
|
|
||
| let hash_copy = hash_for_spawns.clone(); |
There was a problem hiding this comment.
Why isn't this just hash.clone()?
There was a problem hiding this comment.
So the reason why I can't use hash.clone is because rust complains that hash is used by the above transaction, and it is moved into the transactions closure because string does not implement the copy constructor. Because of this I need to create 2 clones of hash, one for the transaction, and one for the tokio spawns (there are 2 of them)
AbrarQuazi
force-pushed
the
update-transaction-read-job
branch
from
c33b2a7 to
f126081
Compare
rust/rsc/src/bin/rsc/read_job.rs
Outdated
| .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.
There was a problem hiding this comment.
sounds good, will add job_id to the failure message
| }; | ||
|
|
||
| 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
There was a problem hiding this comment.
This is the rust compiler error I am getting for just trying to clone hash
No description provided.