Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add priority queue support #38

Merged
merged 2 commits into from
Feb 4, 2025
Merged

Add priority queue support #38

merged 2 commits into from
Feb 4, 2025

Conversation

hylje
Copy link
Contributor

@hylje hylje commented Jan 23, 2025

Use the power of Redis Lua scripting to implement priority queueing by sorting the queue list for each job priority.

While this approach has its limits if the queue is very long, as each insert iterates the entire queue at most twice, it is fully compatible with existing queue consumers who don't need to know about job priorities.

@codecov-commenter
Copy link

codecov-commenter commented Jan 23, 2025

Codecov Report

Attention: Patch coverage is 95.83333% with 7 lines in your changes missing coverage. Please review.

Project coverage is 91.85%. Comparing base (e4443b3) to head (c32aa9c).
Report is 3 commits behind head on master.

Files with missing lines Patch % Lines
minique/models/job.py 80.64% 5 Missing and 1 partial ⚠️
minique_tests/test_priority.py 98.80% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master      #38      +/-   ##
==========================================
+ Coverage   91.11%   91.85%   +0.74%     
==========================================
  Files          25       27       +2     
  Lines         855     1007     +152     
  Branches      115       93      -22     
==========================================
+ Hits          779      925     +146     
- Misses         45       50       +5     
- Partials       31       32       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@hylje hylje force-pushed the priority-queue branch 2 times, most recently from a172a97 to 6b5e448 Compare January 23, 2025 10:18
@hylje hylje requested review from akx and ruksi and removed request for akx January 23, 2025 10:21
@hylje hylje added the orchestration 🎻 Orchestration team tasks label Jan 27, 2025
@hylje hylje force-pushed the priority-queue branch 4 times, most recently from 54e7990 to 3600318 Compare January 29, 2025 12:09
Copy link
Member

@ruksi ruksi left a comment

Choose a reason for hiding this comment

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

looks straightforward 💯

my initial though would've been to keep the public API more or less the same with the priority queues

not highly against the approach, but looking forward to the discussion and if it leads to any changes 😆

README.md Show resolved Hide resolved
minique/api.py Outdated Show resolved Hide resolved
minique/models/job.py Outdated Show resolved Hide resolved
minique/api.py Show resolved Hide resolved
minique/api.py Outdated Show resolved Hide resolved
minique/models/priority_queue.py Outdated Show resolved Hide resolved
minique/models/priority_queue.py Outdated Show resolved Hide resolved
@hylje hylje requested review from ruksi and akx February 4, 2025 10:55
@hylje
Copy link
Contributor Author

hylje commented Feb 4, 2025

thank you for the reviews, updated sources

@@ -81,7 +81,7 @@ jobs:
- run: pip install build twine
- run: python -m build .
- run: twine check dist/*
- uses: actions/upload-artifact@v3
- uses: actions/upload-artifact@v4
Copy link
Member

Choose a reason for hiding this comment

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

sneaky sneak

Copy link
Contributor Author

Choose a reason for hiding this comment

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

required for the CIs to pass

Copy link
Member

@ruksi ruksi left a comment

Choose a reason for hiding this comment

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

excited to see it in action!

@ruksi ruksi merged commit 61610c6 into master Feb 4, 2025
10 checks passed
@ruksi ruksi deleted the priority-queue branch February 4, 2025 11:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
orchestration 🎻 Orchestration team tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants