-
Notifications
You must be signed in to change notification settings - Fork 56
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
Expose the Lock ID on the locks page #127
Expose the Lock ID on the locks page #127
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.
I've not been in a situation where i was interested in the lock id personally. But the reasoning makes sense.
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.
Actually, there is documentation changes missing. And adding a small test to pg_admin.t
also wouldn't be hard.
ee541da
to
4f7873f
Compare
Thanks for the pointers. I have added the changes and added the test. I also took the time to add some tests for the status endpoint for the locks too. |
Changes
Outdated
@@ -1,5 +1,6 @@ | |||
|
|||
10.29 2023-11-28 | |||
- Improved admin ui to show the lock ID on the locks page. |
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.
Don't write your own Changes entries please, those are done by the maintainers.
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.
Instead it needs to be mentioned in the documentation, like the other fields.
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.
you are correct, I have corrected this.
And don't forget to squash your commits when you are ready for review. |
4f7873f
to
01bab6f
Compare
01bab6f
to
183b7ba
Compare
lib/Minion/Backend/Pg.pm
Outdated
id => 1 | ||
|
||
Lock id. | ||
|
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.
There are multiple places where this needs to be added. Also the fields are in alphabetical order.
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.
Updated Backend.pm, I think this was the only other place I neede
Also a perltidy fix is needed. There will be a new Minion release in a few hours, if the mentioned points are resolved this PR can be included. |
183b7ba
to
d1489cf
Compare
d1489cf
to
90cbf56
Compare
ok, PT'ed the test. |
Summary
Expose the lock ID on the Minion admin locks page
Motivation
I employ locks to manage job execution across multiple worker boxes. However, apart from querying the database, I'm unable to determine whether the locks are being rotated. The expiration time decreasing provides some indication, but if the jobs finish within a minute, I can't tell solely from the lock screen whether they are cycling or being held for that minute.
Since it's only retrieving the ID, which would be part of the index, there should be no slowdown in the database.
References
Sorry this is a drive by pull request