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 job releasing back to queue #2180

Closed
wants to merge 1 commit into from

Conversation

lukaszaleckas
Copy link

This pull request fixes job releasing and issue while using queue "--tries" option.

  • Overrides "release" parent function. Uses "findOneAndUpdate" for job releasing (updates queue name, resets "reserved" and "reserved_at" and so on)

  • Overrides "deleteAndRelease" parent function (to skip transactions) - calls "release" method.

@Smolevich
Copy link
Contributor

What is the root bug for creating this fix?

@Smolevich
Copy link
Contributor

What is the root bug for creating this fix?

As i can see it related with branch 8.x of laravel-framework and this changes

@Smolevich Smolevich self-requested a review January 11, 2021 13:00
@lukaszaleckas
Copy link
Author

What is the root bug for creating this fix?

As i can see it related with branch 8.x of laravel-framework and this changes

Yes, you're correct - issue is related to these changes. When you run queue worker with "--tries" option and job fails, "deleteAndRelease" function is called and it uses transaction ("Call to a member function beginTransaction() on null" is thrown). Also, release method on the job calls the same function, so it does not work.

Transaction is used because job is deleted and then reinserted, but honestly I can't find a reason why this is being done instead of simple update on a row. @Smolevich Maybe you know the reason?

@divine
Copy link
Contributor

divine commented Jan 11, 2021

Transaction is used because job is deleted and then reinserted, but honestly I can't find a reason why this is being done instead of simple update on a row

Basically when a simple update fails transaction will revert back changes so no job will be lost.

It's actually a good idea to use transactions but MongoDB is a little bit different for that in terms of requiring sharded/replicas clusters.

Forgot to mention that MongoDB on its own handles this well.

@lukaszaleckas
Copy link
Author

Transaction is used because job is deleted and then reinserted, but honestly I can't find a reason why this is being done instead of simple update on a row

Basically when a simple update fails transaction will revert back changes so no job will be lost.

It's actually a good idea to use transactions but MongoDB is a little bit different for that in terms of requiring sharded/replicas clusters.

Forgot to mention that MongoDB on its own handles this well.

Yeah, transaction is understandable, "delete & insert" part I don't understand. Why not just do update query? In that case you would not even need transactions because it would be only one query instead of two separate.

@divine
Copy link
Contributor

divine commented Jan 12, 2021

Yeah, transaction is understandable, "delete & insert" part I don't understand. Why not just do update query? In that case you would not even need transactions because it would be only one query instead of two separate.

Well, hard to say why they're doing this. 🤔

@divine
Copy link
Contributor

divine commented May 2, 2021

Closed in favor of #2229

Thank you for your contribution @lukaszaleckas !

@divine divine closed this May 2, 2021
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

3 participants