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

gc_worker: Skip TiFlash nodes when doing UnsafeDestroyRange and Green GC #15505

Merged
merged 3 commits into from
Mar 20, 2020

Conversation

MyonKeminta
Copy link
Contributor

@MyonKeminta MyonKeminta commented Mar 19, 2020

What problem does this PR solve?

Issue Number: close #15496

Problem Summary: GC delete range phase and (if green gc is enabled) resolve lock phase cannot successfully run when there are TiFlash nodes in the cluster.

What is changed and how it works?

What's Changed: Filters out TiFlash nodes when sending UnsafeDestroyRange and Green GC related stuff, according to the store's "engine" label. But note that it's possible to be broken when the label is incorrectly configured (see tikv/tikv#7153).

How it Works: For a TiFlash node, it uses other approach to delete dropped tables, so it's safe to skip sending UnsafeDestroyRange requests; it has only learner peers and their data must exist in TiKV, so it's safe to skip physical resolve locks for it. So in theory GC is still correct.

Related changes

  • Need to cherry-pick to the release branch
    • release-3.1 (which supports TiFlash)

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
    • Need to be test if it actually works in a TiFlash-equipped cluster. (Help wanted)

Side effects

Release note

(I'm not sure if this is needed since it's related to TiFlash)

  • Fixes the issue that GC may not work in a cluster that has TiFlash

@codecov
Copy link

codecov bot commented Mar 19, 2020

Codecov Report

Merging #15505 into master will not change coverage by %.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master     #15505   +/-   ##
===========================================
  Coverage   80.4264%   80.4264%           
===========================================
  Files           502        502           
  Lines        134278     134278           
===========================================
  Hits         107995     107995           
  Misses        17806      17806           
  Partials       8477       8477           

@youjiali1995 youjiali1995 added the priority/release-blocker This issue blocks a release. Please solve it ASAP. label Mar 20, 2020
@youjiali1995 youjiali1995 requested a review from sticnarf March 20, 2020 06:56
Copy link
Contributor

@sticnarf sticnarf left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@youjiali1995 youjiali1995 left a comment

Choose a reason for hiding this comment

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

Rest lgtm.

engineLabelTiKV = "tikv"
)

// needsGCOperationForStore checks if the store-level quests related to GC needs to be sent to the store. The store-level
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// needsGCOperationForStore checks if the store-level quests related to GC needs to be sent to the store. The store-level
// needsGCOperationForStore checks if the store-level requests related to GC needs to be sent to the store. The store-level

@MyonKeminta
Copy link
Contributor Author

Manual test goes well.
Master:
image

This branch:
image

@MyonKeminta MyonKeminta added status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. labels Mar 20, 2020
@sre-bot
Copy link
Contributor

sre-bot commented Mar 20, 2020

/run-all-tests

@sre-bot sre-bot merged commit af5ebef into pingcap:master Mar 20, 2020
sre-bot pushed a commit to sre-bot/tidb that referenced this pull request Mar 20, 2020
@sre-bot
Copy link
Contributor

sre-bot commented Mar 20, 2020

cherry pick to release-3.1 in PR #15516

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/GC priority/release-blocker This issue blocks a release. Please solve it ASAP. status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GC delete range doesn't work when the cluster has TiFlash
4 participants