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.

Sign up for GitHub

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

Jump to bottom

Expose timeout for lock via environment variable configuration #786

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Expose timeout for lock via environment variable configuration #786

wants to merge 1 commit into from

Conversation

pmm4654
Copy link

@pmm4654 pmm4654 commented Dec 19, 2023 •
edited
Loading

Expose the timeout configuration option used in Resque::Scheduler::Locking::Base

Related issue: #700

@pmm4654 pmm4654 force-pushed the master branch 3 times, most recently from df7ef73 to 1f9d5cf Compare December 19, 2023 20:02
@pmm4654 pmm4654 marked this pull request as ready for review December 19, 2023 20:02
@learoyklinginsmith
Copy link

@PatrickTulskie any chance we could get some eyes on this change? 🙏

@PatrickTulskie PatrickTulskie self-requested a review April 26, 2024 13:11
PatrickTulskie
PatrickTulskie requested changes May 8, 2024
View reviewed changes
Copy link
Contributor

@PatrickTulskie PatrickTulskie left a comment

Choose a reason for hiding this comment

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

Overall good. I'd just like to set the env stuff the way it was instead of the meta programming approach. Calling send usually winds up causing a lot more scrutiny.

lib/resque/scheduler/env.rb

c.logfile = options[:logfile] if options.key?(:logfile)

c.logformat = options[:logformat] if options.key?(:logformat)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we just leave this and add the new timeout option?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PatrickTulskie PatrickTulskie PatrickTulskie requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@pmm4654 @learoyklinginsmith @PatrickTulskie