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

Update Raft election, lease intervals, etc. #16789

Merged
merged 1 commit into from
May 1, 2023

Conversation

rmloveland
Copy link
Contributor

Fixes DOC-6375, DOC-7125

Summary of changes:

  • Update 'Distribution Layer' page with updated network timeout and mention the COCKROACH_NETWORK_TIMEOUT env var that controls it

  • Update 'Replication Layer' page with:

    • A table listing all of the relevant intervals that can be controlled with the COCKROACH_RAFT_ELECTION_TIMEOUT_TICKS variable

    • More links to the table wherever the values in that table are referred to

  • Finally, all of the intervals are stored in variables so their values can be referenced and updated in one place going forward

@rmloveland
Copy link
Contributor Author

Hi @erikgrinaker these docs changes are based on your work in cockroachdb/cockroach#92542 and cockroachdb/cockroach#91947

Please let me know if I got some details wrong, I read through your comments in the code PRs but probably didn't understand everything properly

also open to any general feedback/comments on how we're presenting this info, or anything else that comes to mind

thanks!

@netlify
Copy link

netlify bot commented Apr 18, 2023

Netlify Preview

Name Link
🔨 Latest commit 0c486c6
🔍 Latest deploy log https://app.netlify.com/sites/cockroachdb-docs/deploys/64501076f21a720007d012e4
😎 Deploy Preview https://deploy-preview-16789--cockroachdb-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

Copy link
Contributor

@erikgrinaker erikgrinaker left a comment

Choose a reason for hiding this comment

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

Thanks for updating this!

v23.1/architecture/distribution-layer.md Outdated Show resolved Hide resolved
v23.1/architecture/replication-layer.md Outdated Show resolved Hide resolved
_data/constants.yml Outdated Show resolved Hide resolved
_data/constants.yml Outdated Show resolved Hide resolved
@rmloveland rmloveland force-pushed the 20230418-raft-election-timeouts-lease-intervals branch from 2319990 to 73d0b91 Compare April 19, 2023 19:12
@rmloveland
Copy link
Contributor Author

@erikgrinaker this is updated based on your previous comments and should be RFAL

@erikgrinaker
Copy link
Contributor

Sorry for the hold-up, I just got out of a non-stop L2 rotation and am still working my way through the backlog. Will get back to you shortly.

Copy link
Contributor

@erikgrinaker erikgrinaker left a comment

Choose a reason for hiding this comment

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

LGTM once comments are addressed. Thanks again for updating this!

I will say that tuning these values isn't trivial, since there are several interactions and aspects to consider. We can write something up around this if you'd like, or we can just consider this to be for advanced users who have some idea of the implications.

@rmloveland
Copy link
Contributor Author

rmloveland commented May 1, 2023

LGTM once comments are addressed. Thanks again for updating this!

NP! Thanks for the thorough review comments, really appreciate it!

I will say that tuning these values isn't trivial, since there are several interactions and aspects to consider. We can write something up around this if you'd like, or we can just consider this to be for advanced users who have some idea of the implications.

Ya this seems pretty scary overall from a user POV. Perhaps we can write more about this in the future if it becomes necessary.

I'm pretty torn about having this kind of possibly footgun-adjacent stuff in the docs, but the new ICP is supposed to be pretty smart 'n stuff so I guess it's fine? My personal (perhaps incorrect) opinion is that if you're sophisticated enough to be tweaking your Raft timeouts etc you should probably be pretty deep in the actual code, so these docs might be redundant or just an attractive nuisance to the less skilled. But hey let's go with it and see what happens! :-)

@rmloveland rmloveland requested review from ianjevans and removed request for ianjevans May 1, 2023 16:01
@rmloveland rmloveland force-pushed the 20230418-raft-election-timeouts-lease-intervals branch from fa79111 to 919dd93 Compare May 1, 2023 16:06
@rmloveland rmloveland requested a review from ianjevans May 1, 2023 16:07
@ianjevans
Copy link
Contributor

attractive nuisance

My absolute favorite legal term.

Fixes DOC-6375, DOC-7125

Summary of changes:

- Update 'Distribution Layer' page with updated network timeout and
  mention the `COCKROACH_NETWORK_TIMEOUT` env var that controls it

- Update 'Replication Layer' page with:

  - A table listing all of the relevant intervals, their values, and how
    they can controlled (if possible)

  - More links to the table wherever the values in that table are referred to

- Finally, all of the intervals and other constants are stored in
  variables so their values can be referenced and updated in one place
  going forward
@rmloveland rmloveland force-pushed the 20230418-raft-election-timeouts-lease-intervals branch from 919dd93 to 0c486c6 Compare May 1, 2023 19:18
@rmloveland rmloveland merged commit f1ec99a into master May 1, 2023
@rmloveland rmloveland deleted the 20230418-raft-election-timeouts-lease-intervals branch May 1, 2023 19:40
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.

None yet

3 participants