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

cluster/spec: workaround if store IDs are not monotonically assigned #1011

Merged
merged 2 commits into from
Dec 25, 2020

Conversation

AstroProfundis
Copy link
Contributor

What problem does this PR solve?

Workaround of tikv/pd#3303

A case of the issue is at: https://asktug.com/t/topic/66779

What is changed and how it works?

When we iterate over the store list, check if any store is marked as "tombstone" state, if not, use it as the final state of the "host:port" store, otherwise use the state from store that has the max ID.

Sorting the slice should be not necessary as we are iterating the whole list anyway, and are only finding specific item in.

Check List

Tests

  • Manual test (add detailed scripts or steps below)

Code changes

  • Has exported function/method change

Side effects

  • Increased code complexity

Related changes

  • Need to cherry-pick to the release branch

Release notes:

cluster/spec: add workaround to get correct store state when PD is not assigning store ID monotonically

@AstroProfundis AstroProfundis added type/bug Categorizes issue as related to a bug. type/bug-fix Categorizes PR as a bug-fix category/stability Categorizes issue or PR as a stability enhancement. labels Dec 24, 2020
@AstroProfundis AstroProfundis added this to the v1.3.1 milestone Dec 24, 2020
@AstroProfundis AstroProfundis self-assigned this Dec 24, 2020
@ti-chi-bot ti-chi-bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Dec 24, 2020
@AstroProfundis AstroProfundis changed the title cluster/spec: report store state as up if any of the stores is up cluster/spec: workaround if store IDs are not monotonically assigned Dec 24, 2020
@lucklove
Copy link
Member

/lgtm

@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Dec 24, 2020
@codecov-io
Copy link

codecov-io commented Dec 24, 2020

Codecov Report

Merging #1011 (7090aae) into master (d7fed64) will decrease coverage by 6.77%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1011      +/-   ##
==========================================
- Coverage   55.64%   48.87%   -6.78%     
==========================================
  Files         279      278       -1     
  Lines       19743    19711      -32     
==========================================
- Hits        10986     9633    -1353     
- Misses       7044     8534    +1490     
+ Partials     1713     1544     -169     
Flag Coverage Δ
cluster 38.40% <0.00%> (-5.17%) ⬇️
dm ?
integrate 44.06% <0.00%> (-5.86%) ⬇️
playground 20.32% <ø> (ø)
tiup 16.50% <0.00%> (+0.02%) ⬆️
unittest 23.01% <0.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/cluster/spec/tikv.go 57.07% <0.00%> (-0.57%) ⬇️
components/dm/main.go 0.00% <0.00%> (-100.00%) ⬇️
components/dm/spec/bindversion.go 0.00% <0.00%> (-100.00%) ⬇️
components/dm/spec/cluster.go 0.00% <0.00%> (-87.50%) ⬇️
components/dm/spec/logic.go 0.60% <0.00%> (-78.66%) ⬇️
components/cluster/command/check.go 6.27% <0.00%> (-73.27%) ⬇️
pkg/cluster/template/scripts/dm_worker.go 0.00% <0.00%> (-71.43%) ⬇️
pkg/cluster/task/limits.go 0.00% <0.00%> (-68.75%) ⬇️
pkg/cluster/template/scripts/dm_master.go 0.00% <0.00%> (-67.22%) ⬇️
pkg/cluster/task/sysctl.go 0.00% <0.00%> (-66.67%) ⬇️
... and 66 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d7fed64...7090aae. Read the comment docs.

@lucklove
Copy link
Member

/merge

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Dec 25, 2020
@ti-chi-bot
Copy link
Member

This pull request has been accepted and is ready to merge.

Commit hash: 7090aae

@ti-chi-bot ti-chi-bot merged commit 8df699f into pingcap:master Dec 25, 2020
@AstroProfundis AstroProfundis deleted the fix-3303 branch December 25, 2020 09:35
lucklove added a commit that referenced this pull request Dec 31, 2020
…mbstone (#1011)

Co-authored-by: SIGSEGV <gnu.crazier@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category/stability Categorizes issue or PR as a stability enhancement. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. status/can-merge Indicates a PR has been approved by a committer. status/LGT1 Indicates that a PR has LGTM 1. type/bug Categorizes issue as related to a bug. type/bug-fix Categorizes PR as a bug-fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants