8000 perf: use lpop with count and don't fetch upload in Upload task by joseph-sentry · Pull Request #707 · codecov/worker · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content
This repository was archived by the owner on May 5, 2025. It is now read-only.

perf: use lpop with count and don't fetch upload in Upload task #707

Merged
merged 1 commit into from
Sep 16, 2024
Merged

perf: use lpop with count and don't fetch upload in Upload task #707

merged 1 commit into from
Sep 16, 2024

Conversation

joseph-sentry
Copy link
Contributor

we don't need to fetch the report upload because all we're doing is setting the upload_pk to upload_id in the normalized args dict

we also can batch our redis lpops into groups of 50 so we don't have to make as many calls to redis

@joseph-sentry
perf: use lpop with count and don't fetch upload in Upload task
de6065a 
we don't need to fetch the report upload because all we're doing is
setting the upload_pk to upload_id in the normalized args dict

we also can batch our redis lpops into groups of 50 so we don't have
to make as many calls to redis
@joseph-sentry joseph-sentry requested a review from a team September 13, 2024 18:18
@sentry-io Sentry.io
Copy link
sentry-io bot commented Sep 13, 2024

🔍 Existing Issues For Review

Your pull request is modifying functions with the following pre-existing issues:

📄 File: tasks/upload.py

Function Unhandled Issue
run_impl_within_lock MaxRetriesExceededError: Can't retry app.tasks.upload.Upload[fd6a0ae8-01f2-49af-bf6c-c8f1c0e2d30b] args:() kwargs:{'debug'... ...
Event Count: 1

Did you find this useful? React with a 👍 or 👎

@codecov-notifications
Copy link
codecov-notifications bot commented Sep 13, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 82.35294% with 3 lines in your changes missing coverage. Please review.

✅ All tests successful. No failed tests found.

Files Patch % Lines
tasks/tests/unit/test_upload_task.py 75.00% 3 Missing ⚠️

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #707      +/-   ##
==========================================
- Coverage   98.09%   98.08%   -0.02%     
==========================================
  Files         434      434              
  Lines       36607    36616       +9     
==========================================
+ Hits        35911    35916       +5     
- Misses        696      700       +4
Flag Coverage Δ
integration 98.08% <82.35%> (-0.02%) ⬇️
latest-uploader-overall 98.08% <82.35%> (-0.02%) ⬇️
unit 98.08% <82.35%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
NonTestCode 96.01% <100.00%> (-0.01%) ⬇️
OutsideTasks 98.10% <ø> (-0.01%) ⬇️
Files Coverage Δ
tasks/upload.py 96.34% <100.00%> (+0.01%) ⬆️
tasks/tests/unit/test_upload_task.py 99.09% <75.00%> (-0.45%) ⬇️

... and 1 file with indirect coverage changes

@codecov-qa
Copy link
codecov-qa bot commented Sep 13, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 82.35294% with 3 lines in your changes missing coverage. Please review.

Project coverage is 98.08%. Comparing base (e867451) to head (de6065a).
Report is 4 commits behind head on main.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
tasks/tests/unit/test_upload_task.py 75.00% 3 Missing ⚠️

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #707      +/-   ##
==========================================
- Coverage   98.09%   98.08%   -0.02%     
==========================================
  Files         434      434              
  Lines       36607    36616       +9     
==========================================
+ Hits        35911    35916       +5     
- Misses        696      700       +4
Flag Coverage Δ
integration 98.08% <82.35%> (-0.02%) ⬇️
latest-uploader-overall 98.08% <82.35%> (-0.02%) ⬇️
unit 98.08% <82.35%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
NonTestCode 96.01% <100.00%> (-0.01%) ⬇️
OutsideTasks 98.10% <ø> (-0.01%) ⬇️
Files with missing lines Coverage Δ
tasks/upload.py 96.34% <100.00%> (+0.01%) ⬆️
tasks/tests/unit/test_upload_task.py 99.09% <75.00%> (-0.45%) ⬇️

... and 1 file with indirect coverage changes

@codecov-public-qa Codecov Public QA
Copy link
codecov-public-qa bot commented Sep 13, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 82.35294% with 3 lines in your changes missing coverage. Please review.

Project coverage is 98.08%. Comparing base (e867451) to head (de6065a).

✅ All tests successful. No failed tests found.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #707      +/-   ##
==========================================
- Coverage   98.09%   98.08%   -0.02%     
==========================================
  Files         434      434              
  Lines       36607    36616       +9     
==========================================
+ Hits        35911    35916       +5     
- Misses        696      700       +4
Flag Coverage Δ
integration 98.08% <82.35%> (-0.02%) ⬇️
latest-uploader-overall 98.08% <82.35%> (-0.02%) ⬇️
unit 98.08% <82.35%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
NonTestCode 96.01% <100.00%> (-0.01%) ⬇️
OutsideTasks 98.10% <ø> (-0.01%) ⬇️
Files Coverage Δ
tasks/upload.py 96.34% <100.00%> (+0.01%) ⬆️
tasks/tests/unit/test_upload_task.py 99.09% <75.00%> (-0.45%) ⬇️

... and 1 file with indirect coverage changes

@codecov Codecov
Copy link
codecov bot commented Sep 13, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 82.35294% with 3 lines in your changes missing coverage. Please review.

Project coverage is 98.13%. Comparing base (e867451) to head (de6065a).
Report is 4 commits behind head on main.

Changes have been made to critical files, which contain lines commonly executed in production. Learn more

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
tasks/tests/unit/test_upload_task.py 75.00% 3 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #707      +/-   ##
==========================================
- Coverage   98.14%   98.13%   -0.01%     
==========================================
  Files         475      475              
  Lines       37962    37971       +9     
==========================================
+ Hits        37257    37263       +6     
- Misses        705      708       +3
Flag Coverage Δ
integration 98.08% <82.35%> (-0.02%) ⬇️
latest-uploader-overall 98.08% <82.35%> (-0.02%) ⬇️
unit 98.08% <82.35%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components 8000 Coverage Δ
NonTestCode 96.11% <100.00%> (+<0.01%) ⬆️
OutsideTasks 98.10% <ø> (-0.01%) ⬇️
Files with missing lines Coverage Δ
tasks/upload.py Critical 97.68% <100.00%> (+<0.01%) ⬆️
tasks/tests/unit/test_upload_task.py 99.09% <75.00%> (-0.45%) ⬇️
Related Entrypoints
run/app.tasks.upload.Upload

Swatinem
Swatinem approved these changes Sep 16, 2024
View reviewed changes
Copy link
Contributor
@Swatinem Swatinem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks great!

@joseph-sentry joseph-sentry added this pull request to the merge queue Sep 16, 2024
Merged via the queue into main with commit 276c39f Sep 16, 2024
23 of 27 checks passed
@joseph-sentry joseph-sentry deleted the joseph/arguments-list branch September 16, 2024 13:56
matt-codecov added a commit to codecov/self-hosted that referenced this pull request Oct 16, 2024
@matt-codecov
update redis to 6.2 in docker-compose
a28d739 
codecov/worker#707 uses an lpop argument that requires Redis 6.2
@matt-codecov matt-codecov mentioned this pull request Oct 16, 2024
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Swatinem Swatinem Swatinem approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@joseph-sentry @Swatinem
0