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

TASK: Use SCAN for redis flush #3387

Merged
merged 1 commit into from
Nov 8, 2024

Conversation

kdambekalns
Copy link
Member

This should speed up flush operations and lower the load on Redis.

Review instructions

The functional tests run flush()

Checklist

  • Code follows the PSR-2 coding style
  • Tests have been created, run and adjusted as needed
  • The PR is created against the lowest maintained branch
  • Reviewer - PR Title is brief but complete and starts with FEATURE|TASK|BUGFIX
  • Reviewer - The first section explains the change briefly for change-logs
  • Reviewer - Breaking Changes are marked with !!! and have upgrade-instructions

This should speed up flush operations and lower the load on Redis.
@kdambekalns kdambekalns self-assigned this Aug 29, 2024
@kdambekalns kdambekalns changed the title TASK: Use scan for redis flush TASK: Use SCAN for redis flush Aug 29, 2024
@Sebobo
Copy link
Member

Sebobo commented Sep 9, 2024

Thx sound good! But what does should speed up exactly mean in numbers or examples?
What was your use case?

I would like to test this thoroughly with some millions of tags in my projects.

@kdambekalns
Copy link
Member Author

We saw high load on a Redis server caused by cache flushes calling KEYS. The fact that KEYS ist warned about in the documentation made me try this, and so far we saw a lower load. I don't have hard numbers, sadly, as I do not have direct access to the metrics…

@Sebobo
Copy link
Member

Sebobo commented Sep 16, 2024

Just to confirm and avoid confusion: This method is only triggered manually in Production correct?
I'm currently trying to come up with a "real" test case for the projects I know.

Copy link
Member

@Sebobo Sebobo left a comment

Choose a reason for hiding this comment

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

I can confirm that with this change, cache entries are still flushed.
Though I can't detect any load or timing changes when testing with some 10.000s of keys.

But the docs mention this to be the better way of doing it, so let's go with it?

@kdambekalns
Copy link
Member Author

kdambekalns commented Sep 16, 2024

Just to confirm and avoid confusion: This method is only triggered manually in Production correct? I'm currently trying to come up with a "real" test case for the projects I know.

In the project this caused issues, we see regular calls to that method. We still need to find out where they come from, to be honest. 🙈

That method is called (only) in AbstractFrontend->flush() and that is called automatically only in Develoment context … with the exception of node publishing, which triggers flush() on Flow_Persistence_Doctrine. Unless something custom in the project causes this, which is probable.

@kdambekalns kdambekalns merged commit 10cdecd into neos:8.3 Nov 8, 2024
10 checks passed
@kdambekalns kdambekalns deleted the task/use-scan-for-redis-flush branch November 8, 2024 11:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants