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

Fixing attempts incrementing: #281

Closed
wants to merge 2 commits into from
Closed

Conversation

mrvtoney
Copy link

In the RabbitMQQueue.php:

  • $message was changed to a class property ($this->message)

In the RabbitMQJob.php:

  • class const ATTEMPT_COUNT_HEADERS_KEY was changed from “attempts_count” to “attempts”

-  was changed to a class property (->message)
- class const ATTEMPT_COUNT_HEADERS_KEY was changed from 'attempts_count' to 'attempts'
@mrvtoney
Copy link
Author

@vyuldashev I can't figure out what errored out for the styleci/pr check. Can you point it out for me?

@vyuldashev
Copy link
Owner

vyuldashev commented Nov 15, 2019

$message was changed to a class property ($this->message)

I don't think it's a good idea. Why did you add this?

@mrvtoney
Copy link
Author

mrvtoney commented Nov 15, 2019

The message was a class property in the RabbitMQJob as well.

However, simply changing the value of the const ATTEMPT_COUNT_HEADERS_KEY did not fix the problem as a whole.

The message's state needed to persist through the process in order to increment. Every time the message was used it was recreated. However, when it was recreated it did not take into consideration or apply the logic for the attempts. Meaning that each time a new message was assembled in pushRaw the was not set.

Debugging the code. I realized that the options array was always empty which prevented attempts from being assigned as a message property.

The body of the message originally holds the attempts. Which is then set in the message property.

@vyuldashev
Copy link
Owner

I just pushed tests for job attempts. And seems like it works like expected: https://travis-ci.org/vyuldashev/laravel-queue-rabbitmq/jobs/613081677

Also, we are using this library in many application in production which somehow have logic with attempts count and everything works.

@mrvtoney
Copy link
Author

That is great to hear.

Yeah. While going through the laravel worker code. I saw that database, redis and sync all utilized attempts.

I modified it in order to maintain consistency with laravel's other queue drivers.

I can change it back and test on my side to see if the change from attempt_counts to attempts.

- Changed attempts back to attempts_count
@mrvtoney
Copy link
Author

@vyuldashev I tested and changed 'attempts' back to 'attempts_count'

@vyuldashev
Copy link
Owner

So why do I need to accept this PR if attempts are incrementing?

@mrvtoney
Copy link
Author

mrvtoney commented Nov 19, 2019

Attempts do not increment without the changes to RabbitMQQueue.php.

It fixes this issue:

@mrvtoney
Copy link
Author

@vyuldashev do you have any other suggestions for my PR?

@vyuldashev
Copy link
Owner

I will review and test it and probably this goes to v10

@vyuldashev
Copy link
Owner

I tried your code, seems like it does not fix #235

@mrvtoney
Copy link
Author

@vyuldashev Can you send me the steps you used to test so I can test again on my side, please?

@vyuldashev
Copy link
Owner

@mrvtoney I used the same code from #235
I think I found a solution but it will break BC. So, I need to work on it and it will go to v10

@vyuldashev vyuldashev closed this Nov 22, 2019
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