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

Try to save actions as unique even when the store doesn't support it #1039

Merged
merged 2 commits into from
Feb 28, 2024

Conversation

lsinger
Copy link
Contributor

@lsinger lsinger commented Feb 27, 2024

This PR changes how we save unique actions in an attempt to at least partially address the issue reported in #1023.

The problem with the Hybrid Store and unique actions is that it doesn't support the atomic operation that would be necessary for guaranteeing uniqueness -- we can't write-lock the store for everyone else, check whether an action exists, and if it doesn't insert a new action, and then free the lock. It's not a relational database.

When saving unique actions, we currently ask the store whether it supports saving unique actions or not. If it doesn't we just save the action. So if the Hybrid Store is active (e.g., because we switch to it every time a plugin is disabled) saving a unique action is downgraded to just a regular save. This way it's possible to have many, many actions scheduled that should have been a single unique one.

So as we cannot guarantee uniqueness, we just insert.

This PR changes the strategy to check for an existing pending action and only inserts if none is found. As the Hybrid Store doesn't provide atomic operations, this is a best-effort strategy rather than a guaranteed uniqueness constraint.

The consequence should be that, in practice, a lot fewer duplicate actions should be generated than previously.

To test, check out this branch, step back a commit, and run the tests -- they'll fail as I have introduced a test that expects the Hybrid Store to support uniqueness. Then checkout this branch again, adding back the commit that changes the strategy. Run the tests again -- they should now pass.

Or, in other words:

  1. git checkout update/try-unique-action-hybrid-store
  2. npm install // will update package-lock.json, please ignore update version in package-lock.json #1038
  3. composer install
  4. git checkout ad63d64
  5. composer run test // this should fail 🔴
  6. git switch -
  7. composer run test // this should succeed 🟢

@lsinger lsinger requested a review from a team February 27, 2024 16:14
@lsinger lsinger self-assigned this Feb 27, 2024
@lsinger lsinger requested review from Konamiman and removed request for a team February 27, 2024 16:14
@Konamiman Konamiman merged commit 3c9b1cc into trunk Feb 28, 2024
32 checks passed
@Konamiman Konamiman deleted the update/try-unique-action-hybrid-store branch February 28, 2024 15:08
@lsinger lsinger added this to the 3.7.3 milestone Mar 18, 2024
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.

2 participants