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

queueMessage speed is too low #17

Open
malas opened this issue Dec 15, 2015 · 10 comments
Open

queueMessage speed is too low #17

malas opened this issue Dec 15, 2015 · 10 comments

Comments

@malas
Copy link

malas commented Dec 15, 2015

In DatabaseSpool::queueMessage do not use

$this->em->flush();

for every message as it drops down performance significantly. Flush should be called from the invoking code (externally):

foreach($messages as $message) {
  $this->getContainer()->get('mailer')->send($message);
}
$this->em->flush();
@dfridrich
Copy link
Contributor

I'd better use flush() in every loop to prevent issues when app stops unexpectedly. If it would flush() after loop, it would send again and again email which have been sent already.

👎

@malas
Copy link
Author

malas commented Apr 6, 2016

You can catch exceptions and delay/remove the message which causes it from resending it.

an option not to make SQL transaction after EVERY message would be a truly better way. Just try to send 1000 messages in one loop and check how much time the flushes consume in comparison with generating message content. no more comments would be needed.

@aistis-
Copy link

aistis- commented Aug 17, 2016

Run into the same problem.

@richsage
Copy link
Contributor

I've started a branch here but won't have time in the next few days to look at it in depth. It marks emails as FAILED. This is very rough & ready code so far :-)

@malas
Copy link
Author

malas commented Sep 6, 2016

@richsage sorry, but how this helps our problem? :)

there is an obvious architecture issue when working with emails and DB. Handling failed emails is a different problem. When working with batch emails it is a blocker.

@richsage
Copy link
Contributor

richsage commented Sep 6, 2016

If an email fails during the send process, your options would be:

  • Ignore it, do nothing and carry on - you're not bothered about knowing the outcome
  • Mark it as failed, let some other process capture this new state, carry on
  • Throw an exception and don't continue

The last option is what happens currently. The middle option is what I have started in my branch.

@malas if you are able to open a PR with your suggested approach, I will happily review but at the moment I don't currently have capacity to work on this change.

@aistis-
Copy link

aistis- commented Sep 6, 2016

What if use DBAL instead of entities? It would give a better performance.

@malas
Copy link
Author

malas commented Sep 6, 2016

@aistis- could you explain more? how we could control the flush moment when using DBAL?

@richsage i will try to make one in some freeish weekend :)

@aistis-
Copy link

aistis- commented Sep 6, 2016

@malas depends on flushing strategy:

  • if flush is done after every email - no control is needed as it is single insert statement;
  • if flush is done in batches - build a batch into a memory and commit it in a single transaction, but as @richsage pointed out, here comes a tricky part to handle failed emails.

The idea would be to avoid using EntityManager::flush() as it flushes all other entities, despite you want it or not 😕

@sampart
Copy link
Owner

sampart commented May 25, 2017

The idea would be to avoid using EntityManager::flush() as it flushes all other entities, despite you want it or not 😕

You can change this behaviour thanks to #21 (although that's not in a release yet - if anyone needs it, do shout).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants