-
Notifications
You must be signed in to change notification settings - Fork 29
Store v2 reports in GCS instead of Redis #960
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #960 +/- ##
==========================================
- Coverage 96.06% 96.06% -0.01%
==========================================
Files 828 828
Lines 19279 19275 -4
==========================================
- Hits 18520 18516 -4
Misses 759 759
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found. 📢 Thoughts on this report? Let us know! |
❌ 2 Tests Failed:
View the top 2 failed tests by shortest run time
To view individual test run time comparison to the main branch, go to the Test Analytics Dashboard |
53e4286 to
09b0993
Compare
|
This PR includes changes to |
|
Test Failures Detected: Due to failing tests, we cannot provide coverage reports at this time. ❌ Failed Test Results:Completed 2658 tests with View the full list of failed testspytest
|
38d2647 to
782fb8a
Compare
matt-codecov
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in future PRs could you put the actual logical changes in their own commit(s) so they're easier to find/review?
does worker already know that the uploads may no longer be in redis? or does that PR still need to be written
it doesn't look like the newer endpoint saves the report to redis in the first place
Yes, I will try :-) One thing kind of led to the other, and I ended up just completely remove the duplicated
Yes, the worker has backwards compatibility code for:
|
| commitid, | ||
| report_type=None, | ||
| report_code=None, | ||
| arguments=None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we use these in Worker?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not yet, but I would like to make the task workflow simpler and make the batching more explicit.
codecov/engineering-team#2618 explains some of my ideas I am working towards.
In particular, I would love to get rid of the Upload -> N * UploadProcessor -> UploadFinisher fanout / map/reduce to a different strategy. Passing the arguments explicitly is a first step towards that.
782fb8a to
71d164a
Compare
|
I rebased this on top of #1006, so the diff should be smaller and focus only on the relevant parts. |
74abbad to
a2e3d61
Compare
adrian-codecov
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved, so this should only be merged after #1006 is merged and you update to that latest Shared yeah? (also approved)
Worker has some dedicated code to move an uploaded report from Redis over to GCS. Instead of having that code, we can just store the report into GCS in the first place. It also forwards all the task arguments directly to the `Upload` task, in addition to storing it in Redis, to be forward compatible to some more planned upload processing improvements.
71d164a to
fc82601
Compare
|
✅ All tests successful. No failed tests were found. 📣 Thoughts on this report? Let Codecov know! | Powered by Codecov |
I changed the code in API to store uploads into GCS directly instead of redis in codecov/codecov-api#960. Since then, the code to download from redis and write to GCS in worker became obsolete, so lets remove it.
I changed the code in API to store uploads into GCS directly instead of redis in codecov/codecov-api#960. Since then, the code to download from redis and write to GCS in worker became obsolete, so lets remove it.
Worker has some dedicated code to move an uploaded report from Redis over to GCS. Instead of having that code, we can just store the report into GCS in the first place.
Additionally, this PR also cleans up the
services.archive, replacing it fully with theshared.api_archive.archive.It also forwards all the task arguments directly to the
Uploadtask, in addition to storing it in Redis, to be forward compatible to some more planned upload processing improvements.Depends on codecov/shared#415