-
Notifications
You must be signed in to change notification settings - Fork 39
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
Closes #102: Make max backoff interval configurable #106
Closes #102: Make max backoff interval configurable #106
Conversation
* Renamed maxBackoffInterval constant * Added MaxBackoffInterval field into Config * Check if duration interval to become a leader is meaningful Fixes operator-framework#102
Hi @caueasantos. Thanks for your PR. I'm waiting for a operator-framework member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/ok-to-test |
Pull Request Test Coverage Report for Build 2170237167
💛 - Coveralls |
@jmrodri Have I done something wrong? The CI errors seems not related to the changes and I'm not too sure how to move forward. What do you suggest? |
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
/remove-lifecycle stale |
@jmrodri Can I get a review of this please? 🙏🏼 |
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.
/lgtm
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.
/lgtm
@caueasantos Thanks for the PR! The change looks good. Just curious on the reason why you are using leader election from this repository? This follows the leader for life approach, which we may deprecate (not sure when) in near future, since SDK has moved to using controller-runtime's leader for lease.
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: joelanford The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
No specific reason 😄 I'm going to take look and see if we can migrate it. Thank you |
Description of the change:
Closes #102
This pull requests aims to give users the ability of specifying custom duration intervals between attempts to become the leader. I tried to keep this change extremely simple exporting a new field to the become's Option.
Motivation for the change:
The 16s hard-coded interval between attempts might cause unnecessary leaderless time during deployment. For example, we have a homemade crons solution responsible for handling jobs scheduling and we noticed a gap of jobs that have not been triggered here and there throughout the day as a result of a few seconds without a leader. Reducing the interval to become a leader should consistently prevent that from happening.