Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[Messenger] Add a Doctrine transport#29007

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

Merged

Conversation

@vincenttouzet
Copy link
Contributor

@vincenttouzetvincenttouzet commentedOct 28, 2018
edited
Loading

QA
Branch?master
Bug fix?no
New feature?yes
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets
LicenseMIT
Doc PRsymfony/symfony-docs#10616
DoctrineBundle PRdoctrine/DoctrineBundle#868

As discussed with@sroze at PHPForum in Paris I've worked on adding a Doctrine transport to the Messenger component.

ActuallyAMQP is the only supported transport and it could be a good thing to support multiple transports. Having a Doctrine transport could help users to start using the component IMHO (Almost all projects use a database).

How it works

The code is splitted betwwen this PR and the one on the DoctrineBundle :doctrine/DoctrineBundle#868

Configuration

To configure a Doctrine transport the dsn MUST have the formatdoctrine://<entity_manager_name> where<entity_manager_name> is the name of the entity manager (usuallydefault)

# config/packages/messenger.yamlframework:messenger:transports:my_transport:"doctrine://default?queue=important"

Table schema

Dispatched messages are stored into a database table with the following schema:

ColumnTypeOptionsDescription
idbigintAUTO_INCREMENT, NOT NULLPrimary key
bodytextNOT NULLBody of the message
headerstextNOT NULLHeaders of the message
queuevarchar(32)NOT NULLHeaders of the message
created_atdatetimeNOT NULLWhen the message was inserted onto the table. (automatically set)
available_atdatetimeNOT NULLWhen the message is available to be handled
delivered_atdatetimeNULLWhen the message was delivered to a worker

Message dispatching

When dispatching a message a new row is inserted into the table. SeeSymfony\Component\Messenger\Transport\Doctrine::publish

Message consuming

The message is retrieved by theSymfony\Component\Messenger\Transport\Doctrine\DoctrineReceiver. It calls theSymfony\Component\Messenger\Transport\Doctrine::get method to get the next message to handle.

Getting the next message

  • Start a transaction
  • Lock the table to get the first message to handle (The lock is done with theSELECT ... FOR UPDATE query)
  • Update the message in database to update the delivered_at columns
  • Commit the transaction

Handling the message

The retrieved message is then passed to the handler. If the message is correctly handled the receiver call theSymfony\Component\Messenger\Transport\Doctrine::ack which delete the message from the table.

If an error occured the receiver call theSymfony\Component\Messenger\Transport\Doctrine::nack method which update the message to set the delivered_at column tonull.

Message requeueing

It may happen that a message is stuck indelivered state but the handler does not really handle the message (Database connection error, server crash, ...). To requeue messages theDoctrineReceiver call theSymfony\Component\Messenger\Transport\Doctrine::requeueMessages. This method update all the message with adelivered_at not null since more than the "redeliver timeout" (default to 3600 seconds)

TODO

sroze, thomasbisignani, and XuruDragon reacted with thumbs up emojiXuruDragon reacted with hooray emojiOskarStark, yceruto, apfelbox, sstok, rpkamp, vtsykun, ping-localhost, mnavarrocarter, weaverryan, oallain, and 9 more reacted with heart emoji
Copy link
Contributor

@TaluuTaluu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Looks good, but overall missing typehints / return hints as it's a feature for 4.3+, so php 7.1+. I'm guessing the missing tests are because it's a wip :}

@vincenttouzetvincenttouzetforce-pushed themessenger-doctrine-transport branch from536ed49 to4fae182CompareOctober 29, 2018 15:43
@nicolas-grekasnicolas-grekas added this to thenext milestoneOct 29, 2018
@vincenttouzetvincenttouzetforce-pushed themessenger-doctrine-transport branch 3 times, most recently fromd174c1b to4023eaaCompareOctober 29, 2018 21:44
@vincenttouzetvincenttouzetforce-pushed themessenger-doctrine-transport branch fromcdcd762 to2a31b47CompareMarch 30, 2019 16:16
Copy link
Contributor

@srozesroze left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Can you add@experimental in 4.3 on all these classes please? Then, a few remaining comments. After this, it should be ready to go!

'body' => '{"message": "Hi handled"}',
'headers' => json_encode(['type' => DummyMessage::class]),
'queue' => 'default',
'created_at' => '2019-03-15 12:00:00',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Let's do this then. It's going to beYYYY-MM-DDTHH:mm:ss.sssZ, with theDateTime as UTC. I suggest to use a function rather than having the->format(...) call copy/pasted.

@sroze
Copy link
Contributor

@vincenttouzet mind the conflict withmaster. Once you've removed theid_strategy, can you squash and rebase as well?

@vincenttouzetvincenttouzetforce-pushed themessenger-doctrine-transport branch 2 times, most recently fromb2bccd7 tof4f4bf4CompareMarch 31, 2019 13:15
@vincenttouzet
Copy link
ContributorAuthor

@sroze I've rebase and squash my commits :)

@sroze
Copy link
Contributor

@vincenttouzet but you kept theid_strategy 😉

@vincenttouzetvincenttouzetforce-pushed themessenger-doctrine-transport branch fromf4f4bf4 to3991088CompareMarch 31, 2019 13:21
@vincenttouzet
Copy link
ContributorAuthor

That was just the doc in the configuration (part of another commit).

sroze reacted with thumbs up emoji

returnnull;
}

$doctrineEnvelope['headers'] = Type::getType(Type::JSON)->convertToPHPValue($doctrineEnvelope['headers'],$this->driverConnection->getDatabasePlatform());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Type::JSON does not exists on early Doctrine versionsas thelow test suite is highlighting. Therefore, I suggest to move it back toSTRING (like thebody) and usejson_encode. Whoever wants to have the table with aJSON field can create it manually. (FYI@weaverryan)

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Yes i saw that. I was trying to require a minimum version for Doctrine but the Json type is available since v2.6.0 (tagged 2017-07-23 01:02) ... I've put back the json_encode / json_decode with the string type then

weaverryan reacted with thumbs up emoji
@sroze
Copy link
Contributor

Thank you@vincenttouzet.

weaverryan, vincenttouzet, vtsykun, and onEXHovia reacted with hooray emoji

@srozesroze merged commit88d008c intosymfony:masterMar 31, 2019
sroze added a commit that referenced this pull requestMar 31, 2019
This PR was merged into the 4.3-dev branch.Discussion----------[Messenger] Add a Doctrine transport| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | no| New feature?  | yes| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets || License       | MIT| Doc PR        |symfony/symfony-docs#10616| DoctrineBundle PR |doctrine/DoctrineBundle#868As discussed with@sroze at PHPForum in Paris I've worked on adding a Doctrine transport to the Messenger component.Actually `AMQP` is the only supported transport and it could be a good thing to support multiple transports. Having a Doctrine transport could help users to start using the component IMHO (Almost all projects use a database).# How it worksThe code is splitted betwwen this PR and the one on the DoctrineBundle :doctrine/DoctrineBundle#868## ConfigurationTo configure a Doctrine transport the dsn MUST have the format `doctrine://<entity_manager_name>` where `<entity_manager_name>` is the name of the entity manager (usually `default`)```yml        # config/packages/messenger.yaml        framework:            messenger:                transports:                    my_transport: "doctrine://default?queue=important"```## Table schemaDispatched messages are stored into a database table with the following schema:| Column       | Type     | Options                  | Description                                                       ||--------------|----------|--------------------------|-------------------------------------------------------------------|| id           | bigint   | AUTO_INCREMENT, NOT NULL | Primary key                                                       || body         | text     | NOT NULL                 | Body of the message                                               || headers      | text     | NOT NULL                 | Headers of the message                                            || queue      | varchar(32)     | NOT NULL                 | Headers of the message                                            || created_at   | datetime | NOT NULL                 | When the message was inserted onto the table. (automatically set) || available_at       | datetime   | NOT NULL                 | When the message is available to be handled                      || delivered_at | datetime | NULL                     | When the message was delivered to a worker                        |## Message dispatchingWhen dispatching a message a new row is inserted into the table. See `Symfony\Component\Messenger\Transport\Doctrine::publish`## Message consumingThe message is retrieved by the `Symfony\Component\Messenger\Transport\Doctrine\DoctrineReceiver`. It calls the `Symfony\Component\Messenger\Transport\Doctrine::get` method to get the next message to handle.### Getting the next message* Start a transaction* Lock the table to get the first message to handle (The lock is done with the `SELECT ... FOR UPDATE` query)* Update the message in database to update the delivered_at columns* Commit the transaction### Handling the messageThe retrieved message is then passed to the handler. If the message is correctly handled the receiver call the `Symfony\Component\Messenger\Transport\Doctrine::ack` which delete the message from the table.If an error occured the receiver call the `Symfony\Component\Messenger\Transport\Doctrine::nack` method which update the message to set the delivered_at column to `null`.## Message requeueingIt may happen that a message is stuck in `delivered` state but the handler does not really handle the message (Database connection error, server crash, ...). To requeue messages the `DoctrineReceiver` call the `Symfony\Component\Messenger\Transport\Doctrine::requeueMessages`. This method update all the message with a  `delivered_at` not null since more than the "redeliver timeout" (default to 3600 seconds)# TODO- [x] Add tests- [x] Create DOC PR- [x] PR on doctrine-bundle for transport factory- [x] Add a `available_at` column- [x] Add a `queue` column- [x] Implement the retry functionnality : See#30557- [x] Rebase after#29476Commits-------88d008c [Messenger] Add a Doctrine transport
@vincenttouzet
Copy link
ContributorAuthor

@sroze while updating the Doc PR I realized that I've keep theloop_sleep option but this is not used. I can make another PR to remove it

@sroze
Copy link
Contributor

@vincenttouzet sure.

@vincenttouzet
Copy link
ContributorAuthor

vincenttouzet commentedMar 31, 2019
edited
Loading

@sroze Hum there is also a problem with the DoctrineTransportFactory:

Fatal error: Declaration of Symfony\Component\Messenger\Transport\Doctrine\DoctrineTransportFactory::createTransport(string $dsn, array $options): Symfony\Component\Messenger\Transport\TransportInterface must be compatible with Symfony\Component\Messenger\Transport\TransportFactoryInterface::createTransport(string $dsn, array $options, Symfony\Component\Messenger\Transport\Serialization\SerializerInterface $serializer): Symfony\Component\Messenger\Transport\TransportInterface in /var/www/sources/symfony/src/Symfony/Component/Messenger/Transport/Doctrine/DoctrineTransportFactory.php on line 26

it worked before merging 🤔 . I can fix this in the same MR if you want

@sroze
Copy link
Contributor

I fixed it in#30799.

vincenttouzet reacted with thumbs up emoji

sroze added a commit that referenced this pull requestMar 31, 2019
…terface (sroze)This PR was merged into the 4.3-dev branch.Discussion----------[Messenger] Fix the Doctrine transport to use the new interface| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | yes| New feature?  | no| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets |#29007| License       | MIT| Doc PR        | øCommits-------75e3355 Fix the Doctrine transport to use the new interface
@vincenttouzet
Copy link
ContributorAuthor

@sroze Ok. The PR to remove the unused option is created :#30803

@vincenttouzetvincenttouzet deleted the messenger-doctrine-transport branchApril 7, 2019 07:44
@nicolas-grekasnicolas-grekas modified the milestones:next,4.3Apr 30, 2019
@fabpotfabpot mentioned this pull requestMay 9, 2019
weaverryan added a commit to symfony/symfony-docs that referenced this pull requestMay 24, 2019
…uzet)This PR was submitted for the master branch but it was merged into the 4.3 branch instead (closes#10616).Discussion----------[Messenger] Describe the doctrine transportAdd documentation relative to the PRsymfony/symfony#29007TODO- [ ] Document the max granularity in seconds for delay (doctrine/dbal#2873)Commits-------27c0f9d [Messenger] Describe the doctrine tranport
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@fabpotfabpotfabpot left review comments

@weaverryanweaverryanweaverryan approved these changes

@ycerutoycerutoyceruto left review comments

+7 more reviewers

@TaluuTaluuTaluu requested changes

@ostroluckyostroluckyostrolucky requested changes

@jean-pasqualinijean-pasqualinijean-pasqualini left review comments

@rpkamprpkamprpkamp left review comments

@onEXHoviaonEXHoviaonEXHovia left review comments

@vtsykunvtsykunvtsykun left review comments

@srozesrozesroze approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

4.3

Development

Successfully merging this pull request may close these issues.

16 participants

@vincenttouzet@nicolas-grekas@chalasr@planeth44@sstok@weaverryan@sroze@fabpot@Taluu@ostrolucky@jean-pasqualini@rpkamp@yceruto@onEXHovia@vtsykun@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp