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

Fix for padding in OrderAndCheckPartitions #8873

Merged
merged 3 commits into from
Sep 24, 2021

Conversation

rafael
Copy link
Member

@rafael rafael commented Sep 23, 2021

Description

We ran into an issue similar to the one in #8296. While trying to switch traffic we found the following error:

E0922 08:33:05.346443    2565 main.go:67] E0922 15:33:05.346176 traffic_switcher.go:470] switchShardReads failed: partial result: non-contiguous KeyRange values for RDONLY 

This PR has three changes:

  • adds a new function to the key package so it can check for contagious key ranges. It makes sure this function accounts for padding (in the same way as other key ranges comparison functions).
  • Starts using that function in OrderAndCheckPartitions.
  • Refactors some code duplication in the tests.

Related Issue(s)

Checklist

  • [] Should this PR be backported?
    I don't know. Maybe?? I think this will only affect Slack. So I don't think there is a big pressure to back port this change.
  • Tests were added or are not required
  • Documentation was added or is not required

Deployment Notes

* We ran into some issues related to padding. This is another form of the bug
fixed in here: vitessio#8296

Signed-off-by: Rafael Chacon <rafael@slack-corp.com>
* Better description for the documentation.
* For completeness, compare nil ranges in a similar a way as in the other functions

Signed-off-by: Rafael Chacon <rafael@slack-corp.com>
Copy link
Contributor

@ajm188 ajm188 left a comment

Choose a reason for hiding this comment

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

one minor copy-paste fix, then lgtm

go/vt/key/key_test.go Outdated Show resolved Hide resolved
…package

naming conventions

Signed-off-by: Rafael Chacon <rafael@slack-corp.com>
Copy link
Member

@deepthi deepthi left a comment

Choose a reason for hiding this comment

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

LGTM.
Is it possible that there are other places we could be doing this wrong? Wondering if you audited the code.

@deepthi deepthi merged commit b966492 into vitessio:main Sep 24, 2021
ajm188 pushed a commit to tinyspeck/vitess that referenced this pull request Sep 28, 2021
Fix for padding in OrderAndCheckPartitions
vmogilev pushed a commit to tinyspeck/vitess that referenced this pull request Jun 6, 2022
Fix for padding in OrderAndCheckPartitions
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.

4 participants