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

BUGFIX: Adjust DoctrineEventStorage commit retry attempts #289

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

kdambekalns
Copy link
Member

@kdambekalns kdambekalns commented Mar 17, 2021

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.

@kdambekalns kdambekalns requested a review from bwaidelich March 17, 2021 21:40
@kdambekalns kdambekalns self-assigned this Mar 17, 2021
@paxuclus
Copy link
Contributor

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
Copy link
Member Author

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
Copy link
Contributor

@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
Copy link
Member

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
Copy link
Contributor

@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
Copy link
Member

After all, the commit is from early 2020 :)

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

@paxuclus
Copy link
Contributor

@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
Copy link
Member Author

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

@albe
Copy link
Member

albe commented Mar 19, 2021

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?

@bwaidelich bwaidelich removed their request for review January 17, 2024 14:44
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

Successfully merging this pull request may close these issues.

4 participants