fix: make pb image downloads random, remove prewarm from pb protocol #2784
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Claude finished @MasterPtato's task —— View job PR Review Complete ✅Reviewing the pull request changes and providing feedback on:
Review FeedbackOverviewThis PR removes the image prewarming functionality from the pegboard protocol and changes image download URL selection from deterministic (hash-based) to random shuffling. The changes are generally well-implemented but have some considerations. Code Quality & Best Practices ✅Positive aspects:
Improvement needed:
Potential Bugs & Issues
|
Deploying rivet with
|
| Latest commit: |
2c3eaf4
|
| Status: | 🚫 Build failed. |
MasterPtato
force-pushed
the
07-21-fix_clean_up_worker_threads_in_the_event_of_an_error
branch
from
d29d739 to
17d427c
Compare
MasterPtato
force-pushed
the
07-22-fix_make_pb_image_downloads_random_remove_prewarm_from_pb_protocol
branch
from
68b94a4 to
2c3eaf4
Compare
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| @@ -280,6 +280,10 @@ impl ImageDownloadHandler { | |||
| let image_size = self.download_inner(ctx, image_config).await?; | |||
| let download_duration = download_start_instant.elapsed().as_secs_f64(); | |||
|
|
|||
| crate::metrics::DOWNLOAD_IMAGE_RATE | |||
| .with_label_values(&[&(start_instant.elapsed().as_nanos() % 100).to_string()]) | |||
There was a problem hiding this comment.
There appears to be a variable name inconsistency in this code. The metric is using image_download_size which isn't defined in the current scope, while the function call on line 280 returns a value stored in image_size. To fix the compilation error, please update the metric to use the correct variable:
crate::metrics::DOWNLOAD_IMAGE_RATE
.with_label_values(&[&(start_instant.elapsed().as_nanos() % 100).to_string()])
.set(image_size as f64 / download_duration);Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
| if let Err(err) = client | ||
| .get(format!("http://{}:8080{}", lan_ip, &artifact_url_stub)) | ||
| .send() | ||
| .await | ||
| .and_then(|res| res.error_for_status()) | ||
| { | ||
| tracing::error!( | ||
| ?err, | ||
| server_id=?server.server_id, | ||
| build_id=?build.build_id, | ||
| "failed prewarming", | ||
| ); | ||
| } | ||
| } | ||
| }) | ||
| .buffer_unordered(16) | ||
| .collect::<()>() | ||
| .await; |
There was a problem hiding this comment.
The current implementation silently continues execution even if all prewarming HTTP requests fail. While errors are logged, there's no mechanism to report the overall success/failure rate or propagate errors to the caller. This could lead to a situation where the system believes prewarming occurred successfully when it actually failed completely.
Consider enhancing the error handling by:
- Tracking successful vs. failed requests
- Returning a meaningful response that indicates the prewarming status
- Potentially implementing retry logic for failed requests
This would provide better visibility into the prewarming process and allow callers to make informed decisions based on the actual outcome.
| if let Err(err) = client | |
| .get(format!("http://{}:8080{}", lan_ip, &artifact_url_stub)) | |
| .send() | |
| .await | |
| .and_then(|res| res.error_for_status()) | |
| { | |
| tracing::error!( | |
| ?err, | |
| server_id=?server.server_id, | |
| build_id=?build.build_id, | |
| "failed prewarming", | |
| ); | |
| } | |
| } | |
| }) | |
| .buffer_unordered(16) | |
| .collect::<()>() | |
| .await; | |
| let result = client | |
| .get(format!("http://{}:8080{}", lan_ip, &artifact_url_stub)) | |
| .send() | |
| .await | |
| .and_then(|res| res.error_for_status()); | |
| match result { | |
| Ok(_) => Ok(()), | |
| Err(err) => { | |
| tracing::error!( | |
| ?err, | |
| server_id=?server.server_id, | |
| build_id=?build.build_id, | |
| "failed prewarming", | |
| ); | |
| Err(()) | |
| } | |
| } | |
| } | |
| }) | |
| .buffer_unordered(16) | |
| .collect::<Vec<Result<(), ()>>>() | |
| .await; | |
| let total_requests = results.len(); | |
| let successful_requests = results.iter().filter(|r| r.is_ok()).count(); | |
| let failed_requests = total_requests - successful_requests; | |
| tracing::info!( | |
| total_requests, | |
| successful_requests, | |
| failed_requests, | |
| "prewarming completed" | |
| ); | |
| PrewarmingResult { | |
| total_requests, | |
| successful_requests, | |
| failed_requests, | |
| } |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
Deploying rivet-studio with
|
| Latest commit: |
2c3eaf4
|
| Status: | 🚫 Build failed. |
Deploying rivet-hub with
|
| Latest commit: |
2c3eaf4
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://201d7371.rivet-hub-7jb.pages.dev |
| Branch Preview URL: | https://07-22-fix-make-pb-image-down.rivet-hub-7jb.pages.dev |
Changes