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

slack-19.0: support consul topo stale reads #559

Open
wants to merge 3 commits into
base: slack-19.0
Choose a base branch
from

Conversation

timvaillancourt
Copy link
Member

@timvaillancourt timvaillancourt commented Nov 15, 2024

Description

This PR allows Vitess components to use stale reads from consul. This is enabled with the flag --topo_allow_stale_reads, which is default false and should be considered experimental

*api.QueryOptions:

// AllowStale allows any Consul server (non-leader) to service
// a read. This allows for lower latency and higher throughput
AllowStale bool

This flag allow us to distribute topo .Get(...) and .List(...) load to consul replicas. Also it limits the impact of elections

It may be safe in use cases like txthrottler if stale reads have no impact to any other vttablet operations. It may be safe to use in vtorc also

Related Issue(s)

Checklist

  • "Backport to:" labels have been added if this change should be back-ported
  • Tests were added or are not required
  • Did the new or modified tests pass consistently locally and on the CI
  • Documentation was added or is not required

Deployment Notes

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
@github-actions github-actions bot added this to the v19.0.7 milestone Nov 15, 2024
@timvaillancourt timvaillancourt marked this pull request as ready for review November 15, 2024 01:23
@timvaillancourt timvaillancourt requested a review from a team as a code owner November 15, 2024 01:23
Copy link
Collaborator

@ejortegau ejortegau left a comment

Choose a reason for hiding this comment

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

LGTM but some CI failed. I triggered a re-run to see if that helps

@timvaillancourt
Copy link
Member Author

timvaillancourt commented Nov 15, 2024

LGTM but some CI failed. I triggered a re-run to see if that helps

@ejortegau the query serving downgrade tests on v19 are known-broken and currently not required CI

For context, we've confirmed we shouldn't be impacted by the handful of failures in those CIs but we still need to modify the pipeline to understand this

Copy link

This PR is being marked as stale because it has been open for 30 days with no activity. To rectify, you may do any of the following:

  • Push additional commits to the associated branch.
  • Remove the stale label.
  • Add a comment indicating why it is not stale.

If no action is taken within 7 days, this PR will be closed.

@github-actions github-actions bot added the Stale label Dec 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants