[Messenger] DoctrineTransport: ensure auto setup is only done once #32308

bendavies · Jul 1, 2019

Q	A
Branch?	4.3
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#...
License	MIT
Doc PR	symfony/symfony-docs#...

Setup is done for every invocation of get, find, and findAll.

For get, this causes message consumption to be very slow, as slow as 1 message per second on my moderately sized schema, because a schema diff is done in setup every get.

I'm not sure what the desired behaviour is here, but it seems like we should try performing a query, fail once, setup, and retry. This will the same approach taken by the PDO Cache.

However, we still need auto setup in get, as get starts a transaction, so the auto setup in executeQuery won't be called.

Further more, the same problem seems to exist for the AMPQ Transport, but the performance impact should be less there, but i have not tried.

src/Symfony/Component/Messenger/Transport/Doctrine/Connection.php

weaverryan · Jul 1, 2019

src/Symfony/Component/Messenger/Transport/Doctrine/Connection.php

@@ -134,7 +136,7 @@ public function send(string $body, array $headers, int $delay = 0): string

    public function get(): ?array
    {
-        if ($this->configuration['auto_setup']) {
+        if ($this->autoSetup) {


maybe we should add a comment here:

// manually try to setup because the transaction below prevents setup on failure

Hmm, or I have a different idea. The truly desired behavior is to only setup once we know that the table is actually missing. That's how it's done on every other method... we just can't do it here because of the transaction.

Could we instead do this:

Don't do any "setup" here - so, make it like all the other methods

In executeQuery(), tweak the logic a little bit inside the } catch (TableNotFoundException $e) {. Specifically, if we are in a transaction, immediately re-throw the TableNotFoundException. Otherwise, do the current logic of setting up and trying again.

In get(), in the catch(), if the exception is a TableNotFoundException, rollback() (like normal), but then setup and then try the entire get() method again (probably we'll need to extra the majority of get() - the part inside the try to a private function so it can be called both in the try and again in the catch).

@vincenttouzet WDYT? By the way, see #32712 where a related conversation (for different reasons) is happening around AMQP.

@weaverryan I like the idea. This way the setup would only be run once for ALL workers. For now it runs at the first get for each worker (if the auto_setup is configured to true)

@bendavies we have some consensus - could you try to put this into your PR? thx :)

@weaverryan sounds good. bit busy this week but can take a look after that. if you want to go ahead without me, feel free.

I agree that we should be able to implement this like ryan suggested. @bendavies do you have time to finish this?

Forgot about this. I'll look to do it next week.

@weaverryan better late than never. take a look.

be careful about doing a setup inside a transaction. In some platforms (MySQL being one of them), doing a DDL statement in a transaction will commit the existing transaction and start a new one after, so a rollback later will rollback only until your DDL statement.

@stof it seems that is explicitly checked for now and handled, isn't it?

src/Symfony/Component/Messenger/Transport/Doctrine/Connection.php

weaverryan · Jul 1, 2019

@vincenttouzet I'd like your eyes on this too :)

Tobion

#32308 (comment)

mareksuscak · Sep 23, 2019

I was wondering if this PR fixes an issue I'm seeing here. If so, is there a workaround? I've disabled auto-setup, yet I'm still seeing tons and tons of queries. They're slamming our database.

EDIT: Yeah, it did work, after all.

weaverryan

This looks great - we setup lazily in executeQuery() in all cases. And for the one "transaction" case in get(), we handle it manually.

The Connection was only ever written the "incorrect" way because we were imitating the logic from Amqp, where the connection setup is cheap.

weaverryan

Hmm, but AppVeyor test failure does look related

bendavies · Sep 27, 2019

fixed?

bendavies · Sep 27, 2019

failure unrelated.

weaverryan

Looks good!

src/Symfony/Component/Messenger/Tests/Transport/Doctrine/DoctrineIntegrationTest.php

src/Symfony/Component/Messenger/Transport/Doctrine/Connection.php

weaverryan · Oct 7, 2019

This should be ready to go!

fabpot · Oct 7, 2019

Thank you @bendavies.

…one once (bendavies) This PR was squashed before being merged into the 4.3 branch (closes #32308). Discussion ---------- [Messenger] DoctrineTransport: ensure auto setup is only done once | Q | A | ------------- | --- | Branch? | 4.3 | Bug fix? | yes | New feature? | no  | BC breaks? | no  | Deprecations? |no  | Tests pass? | yes  | Fixed tickets | #...  | License | MIT | Doc PR | symfony/symfony-docs#...  Setup is done for every invocation of `get`, `find`, and `findAll`. For `get`, this causes message consumption to be very slow, as slow as 1 message per second on my moderately sized schema, because a schema diff is done in `setup` every `get`. I'm not sure what the desired behaviour is here, but it seems like we should try performing a query, fail once, `setup`, and retry. This will the same approach taken by the PDO Cache. However, we still need auto setup in `get`, as `get` starts a transaction, so the auto setup in `executeQuery` won't be called. Further more, the same problem seems to exist for the AMPQ Transport, but the performance impact should be less there, but i have not tried. Commits ------- 0241402 [Messenger] DoctrineTransport: ensure auto setup is only done once

carsonbot added Status: Needs Review Bug labels Jul 1, 2019

bendavies changed the title ~~ensure auto setup once only done once~~ [Messenger] DoctrineTransport: ensure auto setup once only done once Jul 1, 2019

bendavies commented Jul 1, 2019

View reviewed changes

src/Symfony/Component/Messenger/Transport/Doctrine/Connection.php Show resolved Hide resolved

bendavies changed the title ~~[Messenger] DoctrineTransport: ensure auto setup once only done once~~ [Messenger] DoctrineTransport: ensure auto setup is only done once Jul 1, 2019

weaverryan requested changes Jul 1, 2019

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Needs Review labels Jul 1, 2019

weaverryan mentioned this pull request Jul 1, 2019

[Messenger][AQMP] Setup is called every time accessing the queue #32172

Closed

nicolas-grekas added this to the 4.3 milestone Jul 3, 2019

Tobion mentioned this pull request Jul 26, 2019

[Messenger] Retrieve table default options from the SchemaManager #32641

Merged

Tobion suggested changes Jul 26, 2019

View reviewed changes

bendavies mentioned this pull request Sep 12, 2019

Create doctrine messenger transport table on send #33559

Closed

alister mentioned this pull request Sep 21, 2019

[Messenger]: Doctrine transport seems to be generating excessive number of queries #33659

Closed

bendavies requested a review from weaverryan September 23, 2019 11:18

carsonbot added Status: Needs Review and removed Status: Needs Work labels Sep 23, 2019

bendavies requested a review from Tobion September 23, 2019 11:18

bendavies requested a review from stof September 25, 2019 11:52

weaverryan approved these changes Sep 25, 2019

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Sep 25, 2019

weaverryan requested changes Sep 25, 2019

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Reviewed labels Sep 25, 2019

bendavies requested a review from weaverryan September 27, 2019 13:58

carsonbot added Status: Needs Review and removed Status: Needs Work labels Sep 27, 2019

weaverryan approved these changes Sep 27, 2019

View reviewed changes

src/Symfony/Component/Messenger/Tests/Transport/Doctrine/DoctrineIntegrationTest.php Show resolved Hide resolved

carsonbot added Status: Reviewed and removed Status: Needs Review labels Sep 27, 2019

fabpot reviewed Sep 27, 2019

View reviewed changes

src/Symfony/Component/Messenger/Transport/Doctrine/Connection.php Show resolved Hide resolved

fabpot approved these changes Oct 7, 2019

View reviewed changes

[Messenger] DoctrineTransport: ensure auto setup is only done once

0241402

fabpot merged commit 0241402 into symfony:4.3 Oct 7, 2019

fabpot mentioned this pull request Oct 7, 2019

Release v4.3.5 #33891

Merged

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

Uh oh!

[Messenger] DoctrineTransport: ensure auto setup is only done once #32308

[Messenger] DoctrineTransport: ensure auto setup is only done once #32308

Uh oh!

Conversation

bendavies commented Jul 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

weaverryan commented Jul 1, 2019

Uh oh!

Tobion left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mareksuscak commented Sep 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

weaverryan left a comment

Choose a reason for hiding this comment

Uh oh!

weaverryan left a comment

Choose a reason for hiding this comment

Uh oh!

bendavies commented Sep 27, 2019

Uh oh!

bendavies commented Sep 27, 2019

Uh oh!

weaverryan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

weaverryan commented Oct 7, 2019

Uh oh!

fabpot commented Oct 7, 2019

Uh oh!

Uh oh!

bendavies commented Jul 1, 2019 •

edited

Loading

Tobion left a comment •

edited

Loading

mareksuscak commented Sep 23, 2019 •

edited

Loading