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.

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

Add validator for leader lease to be larger than heartbeat interval #25747

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

Conversation

in3xes
Copy link

@in3xes in3xes commented Jan 26, 2025

This PR fixes #1348

It brings in delayed validators to make sure raft_heartbeat_interval_ms is less than leader_lease_duration_ms

@CLAassistant
Copy link

CLAassistant commented Jan 26, 2025

CLA assistant check
All committers have signed the CLA.

@in3xes in3xes marked this pull request as draft January 26, 2025 13:05
@in3xes in3xes marked this pull request as ready for review January 26, 2025 13:05
@in3xes in3xes marked this pull request as draft January 26, 2025 13:05
@in3xes in3xes changed the title Add validator for Leader lease to be larger than heartbeat interval Add validator for leader lease to be larger than heartbeat interval Jan 26, 2025
@in3xes in3xes marked this pull request as ready for review January 29, 2025 02:39
DEFINE_validator(leader_lease_duration_ms,
FLAG_DELAYED_COND_VALIDATOR(
FLAGS_raft_heartbeat_interval_ms <= _value,
"Must be greater than raft_heartbeat_interval_ms"));
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"Must be greater than raft_heartbeat_interval_ms"));
yb::Format("Must be greater than or equal to raft_heartbeat_interval_ms: $0", FLAGS_raft_heartbeat_interval_ms)));

DEFINE_validator(raft_heartbeat_interval_ms,
FLAG_DELAYED_COND_VALIDATOR(
_value <= FLAGS_leader_lease_duration_ms,
"Must be less than leader_lease_duration_ms"));
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"Must be less than leader_lease_duration_ms"));
yb::Format("Must be less than or equal to leader_lease_duration_ms: $0", FLAGS_leader_lease_duration_ms)));

@hari90
Copy link
Contributor

hari90 commented Jan 31, 2025

Can you please try these cases and paste the output in the comment description as a test:

# Success cases
./build/latest/bin/yb-master --raft_heartbeat_interval_ms 100 --leader_lease_duration_ms 100  --version
./build/latest/bin/yb-master --raft_heartbeat_interval_ms 100 --leader_lease_duration_ms 50  --version
./build/latest/bin/yb-master --leader_lease_duration_ms 100 --raft_heartbeat_interval_ms 100  --version
./build/latest/bin/yb-master --leader_lease_duration_ms 50 --raft_heartbeat_interval_ms 100  --version

./build/latest/bin/yb-tserver --raft_heartbeat_interval_ms 100 --leader_lease_duration_ms 100  --version
./build/latest/bin/yb-tserver --raft_heartbeat_interval_ms 100 --leader_lease_duration_ms 50  --version
./build/latest/bin/yb-tserver --leader_lease_duration_ms 100 --raft_heartbeat_interval_ms 100  --version
./build/latest/bin/yb-tserver --leader_lease_duration_ms 50 --raft_heartbeat_interval_ms 100  --version

# Failure cases
./build/latest/bin/yb-master --raft_heartbeat_interval_ms 50 --leader_lease_duration_ms 100  --version
./build/latest/bin/yb-master --leader_lease_duration_ms 100 --raft_heartbeat_interval_ms 50  --version

./build/latest/bin/yb-tserver --raft_heartbeat_interval_ms 50 --leader_lease_duration_ms 100  --version
./build/latest/bin/yb-master --leader_lease_duration_ms 100 --raft_heartbeat_interval_ms 50  --version

Copy link
Contributor

@hari90 hari90 left a comment

Choose a reason for hiding this comment

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

Please change
DEFINE_UNKNOWN_int32(leader_lease_duration_ms
to
DEFINE_NON_RUNTIME_int32(leader_lease_duration_ms

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.

Set Leader lease to be larger than the heartbeat interval
3 participants