neos / Neos.EventSourcing

A library for Event Sourcing and CQRS for Flow projects.
MIT License
44 stars 30 forks source link

BUGFIX: Adjust DoctrineEventStorage commit retry attempts #289

Open kdambekalns opened 3 years ago

kdambekalns commented 3 years ago

With #288 the eventstore table will be write-locked for other processes during commit. To reduce the amount of failed write attempts due to DeadlockExceptions from other processes, the retry attempts are increased. The backoff is reduced to 1.2 so that the maximum interval is only about twice as long as before.

paxuclus commented 3 years ago

Background:

With #288 and #287, the eventstore table will be write-locked for other processes during commit. To reduce the amount of failed write attempts due to DeadlockExceptions from other processes, the retry attempts are increased. The backoff is reduced to 1.2 so that the maximum interval is only about twice as long as before.

kdambekalns commented 3 years ago

Ah, so basically those three are one change? Without #288 no need for #287 and thus not for this one? Then maybe I should combine them into one PR instead?

paxuclus commented 3 years ago

@kdambekalns probably, yes.

288 should not be merged without #287, so those should definitely be in a single PR. #289 is technically not necessary for those changes, but makes more sense when combined with those. I guess a single PR makes sense. 👍

bwaidelich commented 3 years ago

The initial numbers were more or less random. And I'm wondering now: Is there any specific reason for the new numbers? 25 attempts seem a bit much at first sight. Also it won't be "about twice as long as before" since every attempt costs time as well (which is not considered in the backoff calculator).

Having written that.. I really don't know. And I assume the former backoff was too short for you and that's why you changed it!?

paxuclus commented 3 years ago

@bwaidelich yeah, the former backoff was too short. However, the new numbers are also pretty random. We just put those in and they seemed to work so we stuck with them.

I'm actually not even sure if we need that many retries anymore. I guess I'll have to take another look at this. After all, the commit is from early 2020 :)

bwaidelich commented 3 years ago

After all, the commit is from early 2020 :)

Huh? Did we miss it that long or was that in a private fork?

paxuclus commented 3 years ago

@bwaidelich nah, we were stuck on an older release of the eventsourcing package for quite a while so never got around to update those patches. They were in a public fork: https://github.com/netlogix/Neos.EventSourcing/commits/release/sport/Classes

kdambekalns commented 3 years ago

Ok, now this "depends" on #288 (and #287 has been merged into that.)

albe commented 3 years ago

I agree that 25 reattempts seems a bit much. Not sure that's really a necessity in the given case, never came across a scenario where a commit would fail so often and then succeed. Increasing the attempts and reducing the exponent also seems to only flatten the curve, so it's like an optimization for reducing effective latency until the commit succeeds (trying more often and waiting less on each single attempt)?

To reduce the amount of failed write attempts due to DeadlockExceptions from other processes, the retry attempts are increased

Sounds a bit counter-intuitive to me. I would expect more attempts in a tighter loop (together with the table locking) to increase the amount of failed write attempts?