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 the issue that tiflash's data is not clean up when tombstone #768

Merged
merged 1 commit into from
Sep 10, 2020

Conversation

july2993
Copy link
Contributor

@july2993 july2993 commented Sep 9, 2020

What problem does this PR solve?

Fix the issue that tiflash's data is not clean up when tombstone

when tombstone
before this pr:
Screen Shot 2020-09-09 at 4 12 21 PM
after this pr:
Screen Shot 2020-09-09 at 5 24 31 PM

What is changed and how it works?

We use ServicePort in DestroyClusterTombstone to construct id and filter by id,
but the id of instances of the cluster is construct using Port(TCPPort for
tiflash),
so it will not get the instance and clean its data in DestroyClusterTombstone.

Note the GetMainPort() is TCPPort now, change the code to both
using GetMainPort() to avoid inconsistent.

Check List

Tests

  • Manual test (add detailed scripts or steps below)

deploy -> scale-in -> check work as expect

Release notes:

Fix the issue that tiflash's data is not clean up when tombstone

We use ServicePort in DestroyClusterTombstone to construct id and filter by id,
but the id of instances of cluster is contract using Port(TCPPort for
	tiflash),
so it will not get the instance and clean it's data in DestroyClusterTombstone.

Note the GetMainPort() is TCPPort now, change the code to both
using GetMainPort() to avoid inconsistent.
@july2993 july2993 requested a review from lonng September 9, 2020 09:25
@july2993 july2993 requested a review from lucklove September 9, 2020 09:26
@july2993 july2993 added the type/bug-fix Categorizes PR as a bug-fix label Sep 9, 2020
@codecov-commenter
Copy link

Codecov Report

Merging #768 into master will increase coverage by 0.01%.
The diff coverage is 63.53%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #768      +/-   ##
==========================================
+ Coverage   58.76%   58.78%   +0.01%     
==========================================
  Files         257      258       +1     
  Lines       19463    19592     +129     
==========================================
+ Hits        11438    11517      +79     
- Misses       6507     6545      +38     
- Partials     1518     1530      +12     
Flag Coverage Δ
#coverage 58.78% <63.53%> (+0.01%) ⬆️

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

Impacted Files Coverage Δ
components/cluster/command/import.go 20.00% <0.00%> (ø)
components/dm/command/import.go 21.21% <0.00%> (ø)
pkg/cluster/ansible/config.go 0.00% <0.00%> (ø)
pkg/cluster/ansible/dirs.go 3.96% <0.00%> (-0.04%) ⬇️
pkg/cluster/ansible/inventory.go 53.29% <0.00%> (ø)
pkg/cluster/operation/action.go 55.90% <0.00%> (-0.12%) ⬇️
pkg/cluster/operation/operation.go 78.57% <ø> (ø)
pkg/cluster/spec/spec.go 91.17% <ø> (ø)
components/dm/ansible/import.go 63.92% <40.00%> (-0.76%) ⬇️
components/cluster/command/deploy.go 73.58% <50.00%> (ø)
... and 21 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 fb83c60...3098830. Read the comment docs.

Copy link
Contributor

@lonng lonng left a comment

Choose a reason for hiding this comment

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

LGTM

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Sep 10, 2020
@lonng lonng merged commit 7d25cbe into pingcap:master Sep 10, 2020
@july2993 july2993 deleted the flash_des branch September 10, 2020 08:03
lucklove pushed a commit that referenced this pull request Sep 11, 2020
We use ServicePort in DestroyClusterTombstone to construct id and filter by id,
but the id of instances of cluster is contract using Port(TCPPort for
	tiflash),
so it will not get the instance and clean it's data in DestroyClusterTombstone.

Note the GetMainPort() is TCPPort now, change the code to both
using GetMainPort() to avoid inconsistent.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/LGT1 Indicates that a PR has LGTM 1. type/bug-fix Categorizes PR as a bug-fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants