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

fix: queues stop working when there are more than 8k jobs with the same timestamp #550

Merged
merged 1 commit into from Nov 23, 2022

Conversation

compwright
Copy link
Collaborator

Resubmitting #192 from @Tob0t and @mattbornski to rebase and re-run CI checks:

Since there is no progress on #123 I created another PR.

FYI: The commit from @mattbornski was taken.

Since our queues stopped working if there are more than 8k jobs with the same timestamp, we needed a fix asap. I know that the PR was not merged since there could be a further improvements. However as @mattbornski mentioned, better this fix with blocking redis for short time than processing no jobs at all.

We are already using this code on our production system since we needed a fix asap.

Disclaimer: I'm not a lua programmer, so feel free to adapt the PR ;)

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling c53dcc7 on compwright:fixTooManyValueToUnpack into bed66bf on bee-queue:master.

Copy link
Contributor

@bradvogel bradvogel left a comment

Choose a reason for hiding this comment

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

Approving.

It looks like this was approved here but with this concern. However, I agree with the comment in the PR description that it's better to block Redis than to not process at all.

@compwright
Copy link
Collaborator Author

@tmeasday care to add a review before we move on this?

@compwright compwright merged commit 28ca743 into bee-queue:master Nov 23, 2022
beequeueci pushed a commit that referenced this pull request Nov 23, 2022
## [1.4.3](v1.4.2...v1.4.3) (2022-11-23)

### Bug Fixes

* queues stop working when there are more than 8k jobs with the same timestamp ([#550](#550)) ([28ca743](28ca743))
@beequeueci
Copy link
Collaborator

🎉 This PR is included in version 1.4.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

@tmeasday
Copy link
Contributor

Seems reasonable but it would be good to have a test I guess (I'm no expert on how these tests work anymore, or ever really).

@bradvogel
Copy link
Contributor

@tmeasday You're absolutely right. I complete overlooked the fact that there wasn't a test 🤦‍♂️. I think this is OK to skip one for now since it's unlikely the original author is interested in adding one now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants