Skip to content

Lease time modification#22

Open
emsaravanan wants to merge 5 commits into
freshworks-oss:masterfrom
emsaravanan:lease-time-modification
Open

Lease time modification#22
emsaravanan wants to merge 5 commits into
freshworks-oss:masterfrom
emsaravanan:lease-time-modification

Conversation

@emsaravanan
Copy link
Copy Markdown

Fixes # .

Changes proposed on the PR:

@samof76
Copy link
Copy Markdown

samof76 commented Jun 24, 2025

There is on major PR pending, will take this after that.

@samof76
Copy link
Copy Markdown

samof76 commented Jul 23, 2025

@emsaravanan this PR has been merge can you please overlay your changes on top of the new code base?

@samof76 samof76 requested review from Copilot and samof76 July 23, 2025 07:02
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR modifies the leader election configuration for the Redis Failover operator by replacing the default leader election service with a custom configuration. The change allows for explicit control over lease timing parameters rather than relying on default values.

  • Introduces custom leader election lock configuration with specific timing parameters
  • Replaces leaderelection.NewDefault() with leaderelection.New() to use the custom configuration
Comments suppressed due to low confidence (1)

Comment on lines +44 to +45

lockCfg := &leaderelection.LockConfig{
Copy link

Copilot AI Jul 23, 2025

Choose a reason for hiding this comment

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

Inconsistent indentation detected. This line uses spaces for indentation instead of tabs, which doesn't match the surrounding code's indentation style.

Suggested change
lockCfg := &leaderelection.LockConfig{
lockCfg := &leaderelection.LockConfig{

Copilot uses AI. Check for mistakes.
Comment on lines +46 to +48
LeaseDuration: 60 * time.Second,
RenewDeadline: 55 * time.Second,
RetryPeriod: 2 * time.Second,
Copy link

Copilot AI Jul 23, 2025

Choose a reason for hiding this comment

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

[nitpick] Consider defining the timing values as named constants to improve maintainability and make the configuration more explicit. Magic numbers like 60, 55, and 2 should be documented or extracted as constants.

Suggested change
LeaseDuration: 60 * time.Second,
RenewDeadline: 55 * time.Second,
RetryPeriod: 2 * time.Second,
LeaseDuration: leaseDuration,
RenewDeadline: renewDeadline,
RetryPeriod: retryPeriod,

Copilot uses AI. Check for mistakes.
@samof76
Copy link
Copy Markdown

samof76 commented Aug 26, 2025

@pratheep-kumar @Sasidharan-Gopal please review. This looks like good to have feature.

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.

4 participants