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] DoctrineTransport: ensure auto setup is only done once#32308

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
fabpot merged 1 commit intosymfony:4.3frombendavies:messenger-doctrine-transport-setup-once
Oct 7, 2019
Merged

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

fabpot merged 1 commit intosymfony:4.3frombendavies:messenger-doctrine-transport-setup-once
Oct 7, 2019

Conversation

@bendavies
Copy link
Contributor

@bendaviesbendavies commentedJul 1, 2019•
edited
Loading

QA
Branch?4.3
Bug fix?yes
New feature?no
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets#...
LicenseMIT
Doc PRsymfony/symfony-docs#...

Setup is done for every invocation ofget,find, andfindAll.

Forget, 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 insetup everyget.

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 inget, asget starts a transaction, so the auto setup inexecuteQuery 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.

@bendaviesbendavies changed the titleensure auto setup once only done once[Messenger] DoctrineTransport: ensure auto setup once only done onceJul 1, 2019
@bendaviesbendavies changed the title[Messenger] DoctrineTransport: ensure auto setup once only done once[Messenger] DoctrineTransport: ensure auto setup is only done onceJul 1, 2019
publicfunctionget(): ?array
{
if ($this->configuration['auto_setup']) {
if ($this->autoSetup) {
Copy link
Member

Choose a reason for hiding this comment

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

maybe we should add a comment here:

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

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, or I have a different idea. Thetruly 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. InexecuteQuery(), tweak the logic a little bit inside the} catch (TableNotFoundException $e) {. Specifically, if we are in a transaction, immediately re-throw theTableNotFoundException. Otherwise, do the current logic of setting up and trying again.
  3. Inget(), in thecatch(), if the exception is aTableNotFoundException, rollback() (like normal), but then setup andthen try the entireget() method again (probably we'll need to extra the majority ofget() - the part inside thetry 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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

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

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

@weaverryan better late than never. take a look.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

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

@weaverryan
Copy link
Member

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

Copy link
Contributor

@TobionTobion left a comment•
edited
Loading

Choose a reason for hiding this comment

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

@mareksuscak
Copy link

mareksuscak commentedSep 23, 2019•
edited
Loading

I was wondering if this PR fixes an issue I'm seeinghere. 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, itdid work, after all.

Copy link
Member

@weaverryanweaverryan left a comment

Choose a reason for hiding this comment

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

This looks great - we setup lazily inexecuteQuery() in all cases. And for the one "transaction" case inget(), 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.

Copy link
Member

@weaverryanweaverryan left a comment

Choose a reason for hiding this comment

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

Hmm, but AppVeyor test failuredoes look related

@bendavies
Copy link
ContributorAuthor

fixed?

@bendavies
Copy link
ContributorAuthor

failure unrelated.

Copy link
Member

@weaverryanweaverryan 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!

@weaverryan
Copy link
Member

This should be ready to go!

@fabpot
Copy link
Member

Thank you@bendavies.

fabpot added a commit that referenced this pull requestOct 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     <!-- seehttps://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
@fabpotfabpot merged commit0241402 intosymfony:4.3Oct 7, 2019
@fabpotfabpot mentioned this pull requestOct 7, 2019
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@fabpotfabpotfabpot approved these changes

@weaverryanweaverryanweaverryan approved these changes

@TobionTobionAwaiting requested review from Tobion

@stofstofAwaiting requested review from stof

+1 more reviewer

@vincenttouzetvincenttouzetvincenttouzet left review comments

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.

9 participants

@bendavies@weaverryan@mareksuscak@fabpot@stof@Tobion@vincenttouzet@nicolas-grekas@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp