-
-
Notifications
You must be signed in to change notification settings - Fork 798
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
Async Symfony Messenger #681
base: main
Are you sure you want to change the base?
Conversation
Great work, @k-37! Do you think it makes sense to create a new entrypoint for the php-worker:
# ...
environment:
- RUN_MIGRATIONS=false
- CONSUMER_TIME_LIMIT=60
- CONSUMER_LIMIT=10
- CONSUMER_MEMORY_LIMIT=128M
command: ['async'] |
Co-authored-by: Stanislau Kviatkouski <[email protected]>
Thank you for your review @7-zete-7. Since we are documenting needed changes to existing project files my intention was to provide minimal needed modifications of existing files, to make it as simple as possible. What do you think? |
Improved `compose.prod.yaml`
Thank you @dimonx3 for taking interest in this pull request. I have improved instructions for |
The idea of adding a separate entrypoint came from how cleverly the default entrypoint was bypassed (using
It seems to me that such a trick may not be completely clear. It seems that it is necessary to either describe it or move the launch to a separate entrypoint so that this behavior is more obvious. |
Perhaps we should move the check for the symfony-docker/frankenphp/docker-entrypoint.sh Lines 29 to 54 in dc170d9
|
@7-zete-7 thank you for your suggestions. I must admit that I don't quite understand the issue with entrypoint and what improvements should be made. I'm split on the suggestion to move migration check. Is it OK to leave both decisions to project maintainers who are more knowledgeable about the project than me? |
The default entrypoint will only work if the first element in command is
This PR uses |
In my opinion, when using the symfony-docker/frankenphp/docker-entrypoint.sh Lines 51 to 53 in dc170d9
Can you help to choose the appropriate option, @maxhelias? |
@7-zete-7 thank you for the clarification. What do you think about using:
instead of:
|
Thanks for the great examples @k-37! Of these two options, I would prefer the second one. Although it would make sense to just use
|
Regarding your point about
it is started after My preference would be to leave it as it is. But I'm fine with whatever maintainers like better. |
Thank you @daFish for taking interest in this PR. I use Here is relevant line in Here is Do you see any other difference which might be behind your issue? |
@k-37 I wanted to recreate the behaviour but wasn't able to. 😅 I think my comment is no longer relevant. |
Hi @k-37, @7-zete-7, I'm not really in favor of a new entrypoint to stay as lite as possible. I find that the For the I'll try this soon for a better feedback 😉 |
@dunglas Maybe merge? |
Hi all, Any update on this PR? Not sure if it's related but my php service is not healthy anymore with the added code (on the CI) |
Hi @LaurentSanson, Could you please give more details about your service and environment? I would like to reproduce the problem. If you could provide simplified non working example that would be great. |
Hey @k-37 ! Sure, here is some of my actual code :
And here is the error on the CI :
I hope this will help, but once again not 100% sure it's related to the added code but kind of |
Can you SSH into your CI server and get output of the command:
Do you have any problems when running your application with Docker outside of CI, i.e. locally? |
Locally I forgot to add the
In the
PS : if I change
By
I've got this error : |
That looks like a separate issue maybe caused by In
Health check is disabled for Your original error is about Try starting your application locally with these commands and report eventual errors:
I have working example of application using async Symfony messenger. Your compose files look pretty similar to mine. |
Yes if the containers are restarting periodically, we can't use the
Yes I found an error due to migrations, only related to my mistakes, my bad. |
This is proposal to fix issue #539 based on my interpretation of conversation in the issue.