Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[Doctrine][Messenger] Make the messenger doctrine transport scalable#50972
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
This allows for projects to set up the messengerconfig using this constant, making it deterministic,and therefore less prone to errors.
We already have the TransportMessageIdStamp, so theDoctrineReceivedStamp is redundant, as it contains the same dataand its usage is semantically more correct by using theTransportMessageIdStamp.
The current `SELECT ... FOR UPDATE` retrieves rows, using the index tofind and lock the retrieved rows.This means that when we have more than one consumer, whileone consumer is locking those rows, the whole index is locked thusother queries (consumers) will be put on hold.While with a small table this might not be noticeable, as the tablegrows the meddling with the index becomes slower and the otherconsumers have to wait more time, eventually making the MQ inoperable.The `SKIP LOCKED` addition will allow other consumers to query thetable and get messages immediately, ignoring the rows that otherconsumers are locking.Co-authored-by: Alexander Malyk <shu.rick.ifmo@gmail.com>
Currently, the doctrine transport only delivers one message at a time,resulting in low performance and a very high amount of hits to the DB.(one hit per message)With batch delivery, the transport will retrieve several messagesin a single query.Co-authored-by: Alexander Malyk <shu.rick.ifmo@gmail.com>
Thank you very much for this PR.
I think, this should've been three PRs. Because you've changed multiple aspects of the Doctrine transport, the review might take a while. Smaller isolated changes are usually easier to review.
Changing the stamps that the transport attaches to the message is a breaking change. If you can split this into a separate PR, please do that and we discuss the change there.
Very nice. I wonder if it makes sense to contribute support for
Do we have other transports that support batches? Would it makes sense to generalize this feature?
And what version would that be? DBAL supports all MariaDB versions since 10.0, up to 11.0 currently.
😄 |
Hi@derrabus tkx for the review.
How would you separate the commits per PR? make sense?
Will do
Yes, I believe it makes sense, I was eager to get this functionality in the messenger though.
The transport was already prepared for this, cozz the
The SKIP LOCKED clause was introduced in MariaDB 10.6.0, but DBAL supports from |
We usually squash PRs before merging. Since you deliver multiple features with this PR, multiple PRs would make it easier to discuss the implementation of one feature without blocking the merge of another feature.
Since we're targeting 6.4 which is due in November, we're not in a hurry. If you have an idea how to contribute this to DBAL, please go ahead.
I know, but unless I've missed something, this PR would be the first that makes the batch size configurable. But that was just an idea/observation from my side. We can go with your approach and think about generalizing that functionality later.
DBAL does not introduce new platform classes unless they're needed. DBAL 3.7 will ship new ones for MariaDB, but still not the one you need. You can query the database version from the connection though. However, the fact that you'll need a new versioned platform class probably indicates that adding that feature to DBAL is a good idea. |
@derrabus |
Opened up this PR The others will follow up after the DBAL PR is merged |
Relevant changes
This PR has a few commits with small improvements and two commits with relevant changes.
Here are the most significant changes:
Replacement and removal of the DoctrineReceivedStamp
It seems to be redundant because there is already another stamp there being used for the same purpose. Thus, in a commit I replace its usage and in the other commit I remove it. This might be considered a breaking change because the DoctrineReceivedStamp is not marked final, Im not sure, you tell me, these commits can be easily dropped without impact to the rest :)
Add "SKIP LOCKED" to the Mysql8 and Postgres query that retrieves messages
The current
SELECT ... FOR UPDATEretrieves rows, using the index to findand lock the retrieved rows.
This means that when we have more than one consumer, while
one consumer is locking those rows, the whole index is locked thus
other queries (consumers) will be put on hold.
While with a small table this might not be noticeable, as the table
grows the meddling with the index becomes slower and the other
consumers have to wait more time, eventually making the MQ inoperable.
The
SKIP LOCKEDaddition will allow other consumers to query thetable and get messages immediately, ignoring the rows that other
consumers are locking.
Add batch delivery
Currently, the doctrine transport only delivers one message at a time,
resulting in low performance and a very high amount of hits to the DB.
(one hit per message)
With batch delivery, the transport will retrieve several messages in a single query.
Testing
I adjusted the tests, and added a few more cases.
I also tested this in a playground project, sending and receiving 400000 asynchronous messages, which were handled by 2 worker containers, each with 2 consumers.
All messages were processed and there were no duplicates.
These tests were done with mysql, postgres and mssql.
Maria DB was not updated with the
SKIP LOCKEDbecause the engine version that supports it, is not yet supported by Doctrine.Oracle was not updated with
SKIP LOCKEDeither because I could not even connect to the damn thing using my db client.