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

Wakeup worker when there are resources freed up #882

Merged
merged 7 commits into from
May 9, 2024

Conversation

mavahedinia
Copy link
Contributor

Related to the bug reported on Issue#881.

@mavahedinia
Copy link
Contributor Author

I would appreciate if you can help me understand why some tests are failing. @agronholm

@agronholm
Copy link
Owner

Would it be possible for you to add a regression test for this?
As for the other tests failing, I can probably take a look later.

@agronholm
Copy link
Owner

Oh, and also add a changelog entry (credit yourself too).

@mavahedinia
Copy link
Contributor Author

I will look into writing tests over the weekend. Thanks for your fast response!

@agronholm
Copy link
Owner

Any progress on those tests?

@mavahedinia
Copy link
Contributor Author

Sorry man, I am so busy with moving that haven't had any chance to look at it.

@agronholm
Copy link
Owner

One issue I have with this PR is that it needlessly wakes up the job processor whenever a job is released, not just when the processor was previously at capacity. Could you fix that?

@agronholm
Copy link
Owner

But if you're still busy with IRL stuff, let me know and I'll look into fixing it and writing tests.

@coveralls
Copy link

coveralls commented May 9, 2024

Coverage Status

coverage: 92.614%. first build
when pulling ce74e1e on mavahedinia:fix-worker-deadlock
into fad16ec on agronholm:master.

@agronholm
Copy link
Owner

I've added the test and changelog entry on your behalf. Thank you for the contribution!

@agronholm agronholm merged commit 0596db7 into agronholm:master May 9, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants