Skip to content
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

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

Merged

Conversation

@bendavies
Copy link
Contributor

commented 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.

@bendavies bendavies force-pushed the bendavies:messenger-doctrine-transport-setup-once branch from 593eb15 to 591a994 Jul 1, 2019
@bendavies bendavies changed the title ensure auto setup once only done once [Messenger] DoctrineTransport: ensure auto setup once only done once Jul 1, 2019
@bendavies bendavies force-pushed the bendavies:messenger-doctrine-transport-setup-once branch 2 times, most recently from a64abac to 1d7a655 Jul 1, 2019
@bendavies 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
@bendavies bendavies force-pushed the bendavies:messenger-doctrine-transport-setup-once branch from 1d7a655 to c53b94d Jul 1, 2019
@@ -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) {

This comment has been minimized.

Copy link
@weaverryan

weaverryan Jul 1, 2019

Member

maybe we should add a comment here:

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

This comment has been minimized.

Copy link
@weaverryan

weaverryan Jul 1, 2019

Member

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:

  1. Don't do any "setup" here - so, make it like all the other methods
  2. 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.
  3. 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.

This comment has been minimized.

Copy link
@vincenttouzet

vincenttouzet Jul 2, 2019

Contributor

@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)

This comment has been minimized.

Copy link
@weaverryan

weaverryan Jul 3, 2019

Member

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

This comment has been minimized.

Copy link
@bendavies

bendavies Jul 3, 2019

Author Contributor

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

This comment has been minimized.

Copy link
@Tobion

Tobion Jul 26, 2019

Member

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

This comment has been minimized.

Copy link
@bendavies

bendavies Jul 26, 2019

Author Contributor

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

This comment has been minimized.

Copy link
@bendavies

bendavies Sep 23, 2019

Author Contributor

@weaverryan better late than never. take a look.

This comment has been minimized.

Copy link
@stof

stof Sep 23, 2019

Member

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.

This comment has been minimized.

Copy link
@bendavies

bendavies Sep 25, 2019

Author Contributor

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

@weaverryan

This comment has been minimized.

Copy link
Member

commented Jul 1, 2019

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

Copy link
Member

left a comment

@bendavies bendavies force-pushed the bendavies:messenger-doctrine-transport-setup-once branch 5 times, most recently from 2c1a151 to 879f212 Sep 23, 2019
@bendavies bendavies requested a review from weaverryan Sep 23, 2019
@bendavies bendavies requested a review from Tobion Sep 23, 2019
@mareksuscak

This comment has been minimized.

Copy link

commented 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.

@bendavies bendavies force-pushed the bendavies:messenger-doctrine-transport-setup-once branch from 879f212 to 8017645 Sep 25, 2019
@bendavies bendavies force-pushed the bendavies:messenger-doctrine-transport-setup-once branch 10 times, most recently from 3f36d06 to 6a6ea3e Sep 25, 2019
@bendavies bendavies requested a review from weaverryan Sep 27, 2019
@bendavies

This comment has been minimized.

Copy link
Contributor Author

commented Sep 27, 2019

fixed?

@bendavies

This comment has been minimized.

Copy link
Contributor Author

commented Sep 27, 2019

failure unrelated.

Copy link
Member

left a comment

Looks good!

@bendavies bendavies force-pushed the bendavies:messenger-doctrine-transport-setup-once branch from 6a6ea3e to e2f7887 Sep 27, 2019
@weaverryan

This comment has been minimized.

Copy link
Member

commented Oct 7, 2019

This should be ready to go!

@fabpot
fabpot approved these changes Oct 7, 2019
@fabpot fabpot force-pushed the bendavies:messenger-doctrine-transport-setup-once branch from e2f7887 to 0241402 Oct 7, 2019
@fabpot

This comment has been minimized.

Copy link
Member

commented Oct 7, 2019

Thank you @bendavies.

fabpot added a commit that referenced this pull request Oct 7, 2019
…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 <!-- please update src/**/CHANGELOG.md files -->
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? |no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | #...   <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!-- required for new features -->

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
@fabpot fabpot merged commit 0241402 into symfony:4.3 Oct 7, 2019
1 of 3 checks passed
1 of 3 checks passed
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
fabbot.io Your code looks good.
Details
@fabpot fabpot referenced this pull request Oct 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.