Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Properly update retry_last_ts when hitting the maximum retry interval #16156

Merged
merged 4 commits into from Aug 23, 2023

Conversation

deepbluev7
Copy link
Contributor

@deepbluev7 deepbluev7 commented Aug 22, 2023

This was broken in 1.87 when the maximum retry interval got changed from almost infinite to a week (and made configurable).

fixes #16101

Pull Request Checklist

  • Pull request is based on the develop branch
  • Pull request includes a changelog file. The entry should:
    • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
    • Use markdown where necessary, mostly for code blocks.
    • End with either a period (.) or an exclamation mark (!).
    • Start with a capital letter.
    • Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry.
  • Pull request includes a sign off
  • Code style is correct
    (run the linters)

@deepbluev7 deepbluev7 requested a review from a team as a code owner August 22, 2023 12:15
@deepbluev7
Copy link
Contributor Author

Basically the problem is that if the retry ts is older than 8 days, but the retry interval is already 7 days, we would never update the last retry. This solution might cause a few too many writes, but those should only happen if we retry for other reasons, I hope?

@deepbluev7 deepbluev7 marked this pull request as draft August 22, 2023 12:21
@deepbluev7
Copy link
Contributor Author

Actually, converting this to a draft, since this does not work, if the retry interval in the db is bigger than the new max retry interval and the server never was online in between...

@deepbluev7
Copy link
Contributor Author

deepbluev7 commented Aug 22, 2023

Okay, I think this works, but there where clauses confuses me :D

@deepbluev7 deepbluev7 marked this pull request as ready for review August 22, 2023 12:45
@deepbluev7
Copy link
Contributor Author

Grafana dash after those changes at around 12:35:

image

This was broken in 1.87 when the maximum retry interval got changed from
almost infinite to a week (and made configurable).

fixes matrix-org#16101

Signed-off-by: Nicolas Werner <nicolas.werner@hotmail.de>
@MatMaul MatMaul added the X-Release-Blocker Must be resolved before making a release label Aug 22, 2023
@MatMaul
Copy link
Contributor

MatMaul commented Aug 23, 2023

I added a test, and change the fix for something looking more legit to me, since I had trouble understanding exactly how your fix was working @deepbluev7.

@erikjohnston erikjohnston merged commit 19a1cda into matrix-org:develop Aug 23, 2023
37 checks passed
@erikjohnston
Copy link
Member

Ah, didn't spot that the fix has changed. I think the current fix does work, but equally for clarity I think we should add a clause to the upsert to ensure that we do update the last_retry_ts.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
X-Release-Blocker Must be resolved before making a release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Federation sender started spamming requests
4 participants