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

feat: Make the communicate_with_quorum grace period configurable #3075

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

feat: Make the communicate_with_quorum grace period configurable #3075

wants to merge 4 commits into from
+45 −16

Conversation

huitseeker
Copy link
Contributor

@huitseeker huitseeker commented Dec 29, 2024 •
edited
Loading

  • Renamed GRACE_PERIOD to DEFAULT_GRACE_PERIOD and altered its value,
  • added grace period argument in communicate with quorum,
  • added that value to ChainClientOptions and threaded through the Client builder,
  • Refactored the end_time setting logic in the communicate_with_quorum function to improve the handling of unreachable quorum on any key,
  • also corrects a handful of typos (in comments and error messages) as I was reading in the area

Fixes #2836. /cc @afck for review @Twey for context

ma2bd
ma2bd reviewed Dec 29, 2024
View reviewed changes
linera-core/src/updater.rs Outdated Show resolved Hide resolved
ma2bd
ma2bd reviewed Dec 29, 2024
View reviewed changes
linera-core/src/updater.rs Outdated Show resolved Hide resolved
@ma2bd ma2bd requested a review from afck December 29, 2024 18:00
ma2bd
ma2bd reviewed Dec 29, 2024
View reviewed changes
linera-core/src/updater.rs
end_time = Some(Instant::now() + start_time.elapsed().mul_f64(GRACE_PERIOD));
// If it becomes clear that no key can reach a quorum, break early.
if highest_key_score + remaining_votes < committee.quorum_threshold() {
break 'vote_wait;
Copy link
Contributor

Choose a reason for hiding this comment

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

cc @afck for reviewing this part

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense to me. 👍
(An alternative would be to set end_time to now(), but maybe the break is cleaner.)

@ma2bd
Copy link
Contributor

ma2bd commented Dec 29, 2024 •
edited
Loading

Thanks @huitseeker for the PR!

  • One cannot change the protocol behavior (here default value) in a PR called "refactor" (also I think 0.2 is fine for now)
  • Consider using several distinct commits in the PR next time :) to separate the easy stuff from the more serious refactoring
  • Seems like you need to run cargo +nightly fmt

@huitseeker huitseeker force-pushed the communicate_with_validators branch from 024925d to 82b4f4e Compare December 29, 2024 18:41
@huitseeker
Copy link
Contributor Author

huitseeker commented Dec 29, 2024 •
edited
Loading

  • removed typo fixes,
  • split change of DEFAULT_GRACE_PERIOD to another commit (happy to omit that commit, waiting for @afck input).

@huitseeker huitseeker changed the title refactor: Make the communicate_with_quorum grace period configurable feat: Make the communicate_with_quorum grace period configurable Dec 29, 2024
@huitseeker huitseeker force-pushed the communicate_with_validators branch from 82b4f4e to 3e15b10 Compare December 29, 2024 18:44
@huitseeker
Copy link
Contributor Author

huitseeker commented Dec 29, 2024 •
edited
Loading

One cannot change the protocol behavior (here default value) in a PR called "refactor" (also I think 0.2 is fine for now)

Ah, yeah, i had not noticed this was public. I would actually advise switching this low level function to pub(crate).

@huitseeker
feat: Make the communicate_with_quorum grace period configurable
63f7177 
- Renamed `GRACE_PERIOD` to `DEFAULT_GRACE_PERIOD` and altered its value,
- added grace period argument in communicate with quorum,
- Refactored the `end_time` setting logic in the `communicate_with_quorum` function to improve the handling of unreachable quorum on any key,
@huitseeker huitseeker force-pushed the communicate_with_validators branch from 3e15b10 to 4eb98d2 Compare December 29, 2024 18:47
afck
afck approved these changes Dec 30, 2024
View reviewed changes
Copy link
Contributor

@afck afck left a comment

Choose a reason for hiding this comment

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

Looks good to me, but it doesn't actually make it configurable yet, i.e. add a config option.

linera-core/src/updater.rs
end_time = Some(Instant::now() + start_time.elapsed().mul_f64(GRACE_PERIOD));
// If it becomes clear that no key can reach a quorum, break early.
if highest_key_score + remaining_votes < committee.quorum_threshold() {
break 'vote_wait;
Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense to me. 👍
(An alternative would be to set end_time to now(), but maybe the break is cleaner.)

linera-core/src/updater.rs Outdated Show resolved Hide resolved
@huitseeker huitseeker force-pushed the communicate_with_validators branch from 4eb98d2 to 31fdf88 Compare December 30, 2024 17:00
@huitseeker
refactor: Update client initialization with additional grace_period o…
bf02eb5 
…ption

Added grace_perio option to the `ChainClientOptions`, and allowed its setting through the `Client` builder.
@huitseeker huitseeker force-pushed the communicate_with_validators branch from 31fdf88 to bf02eb5 Compare December 30, 2024 17:04
@huitseeker
Copy link
Contributor Author

Thanks for the feedback @ma2bd and @afck. I have:

  • removed the commit that changed the default value of the grace period (now at the original value of 0.2)
  • added a commit that includes the grace_period parameter as part of the ChainClientOptions and threads that value through the Client builder.

afck
afck reviewed Dec 30, 2024
View reviewed changes
Copy link
Contributor

@afck afck left a comment

Choose a reason for hiding this comment

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

Thanks; but that still doesn't make it user-configurable: It doesn't add a CLI option yet. (I think that needs to be added to client_options.rs.)

linera-core/src/client/mod.rs Outdated Show resolved Hide resolved
@huitseeker huitseeker force-pushed the communicate_with_validators branch from 5050e24 to 7e13f49 Compare December 30, 2024 22:10
afck
afck approved these changes Jan 1, 2025
View reviewed changes
Copy link
Contributor

@afck afck left a comment

Choose a reason for hiding this comment

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

No blockers from my side!

linera-core/src/client/mod.rs Outdated Show resolved Hide resolved
linera-client/src/client_options.rs Show resolved Hide resolved
linera-client/src/client_options.rs Outdated Show resolved Hide resolved
@huitseeker huitseeker force-pushed the communicate_with_validators branch from 68e5f41 to aa760e1 Compare January 1, 2025 18:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ma2bd ma2bd ma2bd left review comments

@afck afck afck approved these changes

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

Successfully merging this pull request may close these issues.

Make the communicate_with_quorum grace period configurable.
3 participants
@huitseeker @ma2bd @afck