Skip to content

Raise error when advisory lock cannot be acquired #1

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

Merged
merged 7 commits into from
Mar 20, 2025

Conversation

Farjaad
Copy link
Collaborator

@Farjaad Farjaad commented Mar 17, 2025

Gitlab Issue: https://gitlab.com/jklabsinc/OpsLevel/-/issues/13122

I found traces where jobs fail with a lock timeout after 50s when inserting into entity_node_hierarchy so that means our jobs do fail if the actual insert is taking long.

But there are traces (example) where the job sits for minutes just doing SELECT GET_LOCK until it gets shutdown by sidekiq. The SELECT GET_LOCK comes from the closure_tree gem and the gem always calls with_advisory_lock without any options which means that it waits indefinitely for the lock.

So this MR uses with_advisory_lock! so that it raises an error if we can't get the lock instead of just waiting indefinitely so that the jobs will end up in the dead set if there's a lot of contention. https://github.com/ClosureTree/with_advisory_lock/blob/master/lib/with_advisory_lock/concern.rb#L16

@Farjaad Farjaad self-assigned this Mar 17, 2025
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@Farjaad Farjaad force-pushed the 13122-raise-error-if-we-cant-get-lock branch 2 times, most recently from 398e520 to 5787303 Compare March 17, 2025 20:25
@Farjaad Farjaad force-pushed the 13122-raise-error-if-we-cant-get-lock branch 3 times, most recently from f213213 to d9749ba Compare March 19, 2025 17:54
@Farjaad Farjaad force-pushed the 13122-raise-error-if-we-cant-get-lock branch 2 times, most recently from 81a4b14 to 618ef8a Compare March 19, 2025 19:30
@Farjaad Farjaad force-pushed the 13122-raise-error-if-we-cant-get-lock branch 4 times, most recently from 466b321 to cad9d07 Compare March 19, 2025 21:42
@Farjaad Farjaad force-pushed the 13122-raise-error-if-we-cant-get-lock branch from 83e3782 to a21a037 Compare March 20, 2025 15:28
@Farjaad Farjaad force-pushed the 13122-raise-error-if-we-cant-get-lock branch from a21a037 to 0e6fbf5 Compare March 20, 2025 15:35
@Farjaad Farjaad force-pushed the 13122-raise-error-if-we-cant-get-lock branch from d9249be to ef54cd3 Compare March 20, 2025 15:57
@Farjaad Farjaad force-pushed the 13122-raise-error-if-we-cant-get-lock branch from ef54cd3 to ae7b44f Compare March 20, 2025 16:07
@Farjaad Farjaad merged commit 9194820 into master Mar 20, 2025
3 of 4 checks passed
@Farjaad
Copy link
Collaborator Author

Farjaad commented Mar 20, 2025

ahh I forgot to squash my commits 😢

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.

7 participants