[DoctrineMessenger] [WIP] Postgres LISTEN/NOTIFY improvement proposition for better handling of time limit, multi-queue worker and delayed tasks #47666
Conversation
|
Hey! I think @deguif has recently worked with this code. Maybe they can help review this? Cheers! Carsonbot |
|
@nicolas-grekas Even though this is includes a logic change, shouldn't it be considered a bug fix as at multiple queues (which just means prioritizing messages) is kind of broken at the moment? |
|
Hi Christian,
I'm waiting for someone to say "yes/ok" to the points: 2, 3 (to allow an addition of the getter), 6 (to include the "additional" improvement in the scope of this PR), and 7. When I get an approval (or at least an interest in principle), then I'll go ahead and complete this PR (but I might have some further questions about unit testing, and online documentation). So if it's in your ability to obtain this, then please do. The PR contains a running and working code (but note the "sibling" PR for DoctrineBundle), however at present it only includes the code for problem 1, mentioned in #46862. Feel free to fork the 2 git repositories, and install them via composer in your app. The code is obviously not unit-tested, so it may break between Messenger minor/major updates, but it does work at present. Not much else I can say in that regard. At the very least, I'm not going to invest my time in finishing this PR, if I don't see any interest from the Symfony maintainers. I might at some point write a separate Postgres(Messenger)Transport with the changes mentioned in the github ticket, just for my own use. |
|
@d-ph Unfortunately that's not going to be me as I'm not part of that group. But @nicolas-grekas marked it for the next milestone. Isn't that kind of an confirmation? |
|
Chris, I'm not going to play a "what the poet meant" regarding milestones assignment. I don't think I ask for much, considering the fact that I'm going to be doing the heavy-lifting. |
|
I'm sorry. That wasn't meant as a push but rather as "I just realized, did you see that". I also don't know what it means ether, and hoped you might. |
|
this PR needs to take #47209 into account |
…es with custom consumption priorities. Replay symfony#47209 on top of this code change.
|
@nicolas-grekas This PR was moved to different milestones twice, but the author is still waiting on a confirmation / guidance from you before finalizing the PR. Otherwise I guess this will be moved to the next milestone again 😞 |
|
@nicolas-grekas @fabpot This PR was moved to different milestones again, but the author is still waiting on a confirmation / guidance from you before finalizing the PR. Otherwise I guess this will be moved to the next milestone again. |
This PR at present contains a code change that covers the improvement proposition no. 1, mentioned in #46862.
The code change spans 2 projects:
Comments:
PostgreSqlWaitForPgNotifyOnIdleListener::registerPostgreSqlConnectionCandidate()is an annoying necessity stemming from the fact that there is no way to retrieve theSymfony\Component\Messenger\Bridge\Doctrine\Transport\Connectionout ofDoctrineReceiverandDoctrineTransport. If a getter is added to both of this classes, then the::registerPostgreSqlConnectionCandidate()may be removed, which I would recommend doing if that's ok. It would also remove the necessity of injectingPostgreSqlWaitForPgNotifyOnIdleListenertoDoctrineTransportFactory.Symfony\Component\Messenger\Bridge\Doctrine\Transport\Connection::$queueEmptiedAtis removed. This is a nice side effect of the proposed code change, since the variable was related to an obscure postgres implementation detail.PostgreSqlConnectionnow doesn't override theget()function, which perhaps is a plus.PostgreSqlWaitForPgNotifyOnIdleListener::choosePgNotifyTimeout()is awaiting the "improvement proposition no. 2" implementation, which I'd say is the "killer feature" of the related github ticket.PostgreSqlConnection::'check_delayed_interval'option is no longer used, and will become deprecated. It's expected to be replaced with "improvement proposition no. 2".