-
-
Notifications
You must be signed in to change notification settings - Fork 7.6k
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
feat(content-releases): scheduling in multiple strapi instances #19631
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a minor comment but nice job using the locks. The only issue that I possibly forsee is if the lock is set and Strapi crashes or reboots before we release it.
I'm not 100% what happens but might be useful to test manually as options like redlock (Redis distributed locks) have an auto timeout where if the lock isn't released in a certain amount of time, Redis actually releases it itself (lock TTL)
Btw, cc @markkaylor and @remidej since I saw history was also using node schedule as well as audit logs, less critical there since it's just deleting but it's probably good that all squads are aware and carefully about using node-schedule and to determine if we need locking to prevent race conditions in distributed envs |
Hey @derrickmehaffy, yes talked about this earlier with Alex, and concluded that since both audit logs and history only use cron jobs to delete things, it wouldn't be a problem if that action is tried multiple times |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Thanks for your help on discord for testing. In my opinion your PR description could have had more explicit instructions on how to get setup to test this. For example the steps you took to setup your environment for developing / testing. That being said the test was successful 🥳
- The publish function is really becoming almost unreadable. It would be nice if we could clean it up a bit here. PR's involving this function are quite difficult to review.
…ng-with-multiple-instances
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for cleaning things up a bit! Tested again and looks like it's working. Nice job.
What does it do?
This PR ensures that the publish process locks the release's row, preventing race conditions when multiple Strapi instances are running simultaneously. I recommend reading the documentation added in this PR to gain a better understanding of the problem and how this solution addresses it.
How to test it?
You need to simulate multiple Strapi instances. You can achieve this using any technology of your choice, or simply by running the same Strapi application on multiple terminals with different ports.