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

tikv: make region request can send to flash store #11652

Merged
merged 18 commits into from Sep 25, 2019

Conversation

@lzmhhh123
Copy link
Member

commented Aug 7, 2019

What problem does this PR solve?

Make region request can send to flash store.

What is changed and how it works?

Add global config to reveal tiflash store labels and some flash mark in tikv.Store.

Check List

Tests

  • Unit test
  • Integration test

Code changes

  • Has exported function/method change
  • Has exported variable/fields change
  • Has persistent data change

Side effects

  • Possible performance regression
  • Increased code complexity

Related changes

  • None
@codecov

This comment has been minimized.

Copy link

commented Aug 7, 2019

Codecov Report

Merging #11652 into master will increase coverage by 0.2002%.
The diff coverage is 34.4537%.

@@               Coverage Diff                @@
##             master     #11652        +/-   ##
================================================
+ Coverage   80.9013%   81.1015%   +0.2002%     
================================================
  Files           454        454                
  Lines        100138      98405      -1733     
================================================
- Hits          81013      79808      -1205     
+ Misses        13319      12847       -472     
+ Partials       5806       5750        -56
Copy link
Member

left a comment

maybe need take care #11347...

and take care fail retry logic:

  • kv request fail should not switch and try to flash node
  • flash request fail's retry should not switch to kv node(it seem current code works for this item :D)
  • does tiflush has some return error logic like onRegionError?
store/tikv/region_request.go Outdated Show resolved Hide resolved
@lysu lysu requested a review from coocood Aug 8, 2019
@lzmhhh123

This comment has been minimized.

Copy link
Member Author

commented Aug 8, 2019

@lysu
answer for three suggestion:

  1. OK.
  2. Yes, that's my fault.
  3. RegionErr return must keep same with tikv. I'll confirm this with tiflash team.
config/config.go Outdated Show resolved Hide resolved
store/tikv/region_request.go Outdated Show resolved Hide resolved
@coocood

This comment has been minimized.

Copy link
Member

commented Aug 9, 2019

@lzmhhh123
Seems the logic conflicts with this PR
#11347

config/config_test.go Outdated Show resolved Hide resolved
@lzmhhh123

This comment has been minimized.

Copy link
Member Author

commented Aug 9, 2019

@lysu @coocood @zz-jason Because of some reasons that the hint to access flash doesn't toward the agreement with PM team. This PR needn't merge to master now. After testing with the flash team, I will consider the situation with follower read.

@lzmhhh123 lzmhhh123 closed this Aug 9, 2019
@lzmhhh123 lzmhhh123 removed the priority/P1 label Aug 19, 2019
@lzmhhh123 lzmhhh123 reopened this Aug 19, 2019
@lzmhhh123 lzmhhh123 requested review from lysu, zz-jason, XuHuaiyu and coocood Aug 19, 2019
@lzmhhh123

This comment has been minimized.

Copy link
Member Author

commented Aug 19, 2019

It's ready for review.

@lzmhhh123 lzmhhh123 force-pushed the lzmhhh123:dev/add_theflash_access_flag branch from 67ff8b3 to f73853b Aug 19, 2019
store/tikv/region_cache.go Show resolved Hide resolved
store/tikv/region_cache.go Outdated Show resolved Hide resolved
store/tikv/region_cache.go Outdated Show resolved Hide resolved
store/tikv/region_cache.go Outdated Show resolved Hide resolved
store/tikv/region_cache.go Outdated Show resolved Hide resolved
store/tikv/region_request.go Outdated Show resolved Hide resolved
store/tikv/region_cache.go Outdated Show resolved Hide resolved
config/config.go Outdated Show resolved Hide resolved
config/config.go Outdated Show resolved Hide resolved
config/config_test.go Outdated Show resolved Hide resolved
store/tikv/region_request.go Outdated Show resolved Hide resolved
store/tikv/region_request.go Outdated Show resolved Hide resolved
store/tikv/coprocessor.go Show resolved Hide resolved
store/tikv/region_cache.go Outdated Show resolved Hide resolved
@lzmhhh123 lzmhhh123 force-pushed the lzmhhh123:dev/add_theflash_access_flag branch 3 times, most recently from 82f1f8c to 598f889 Aug 21, 2019
lzmhhh123 added 7 commits Sep 16, 2019
…hhh123/tidb into dev/add_theflash_access_flag
@lzmhhh123 lzmhhh123 requested review from SunRunAway and removed request for SunRunAway Sep 23, 2019
Copy link
Member

left a comment

at last, maybe we should take care switchNextPeer

func (c *RegionCache) switchNextPeer(r *Region, currentPeerIdx int, err error) {

this method will be called after send request failure

if e := s.onSendFail(bo, ctx, err); e != nil {

A request to TiKV leader should not switch current work index to TiFish, if so maybe will lead write request to send to TiFlash.

and A req to TiFlash maybe better not trigger "switch current work"(e.g. one TiFlash node has crashed, but TiKV's leader is work well and keep handling oltp load).

s.storeType = TiKV
for _, label := range store.Labels {
if label.Key == "engine" && label.Value == "tiflash" {
s.storeType = TiFlash

This comment has been minimized.

Copy link
@lysu

lysu Sep 24, 2019

Member

we can break when label.Key == "engine" even if label.Value != "tiflash"

storeType := TiKV
for _, label := range store.Labels {
if label.Key == "engine" && label.Value == "tiflash" {
storeType = TiFlash

This comment has been minimized.

Copy link
@lysu

lysu Sep 24, 2019

Member

ditto

regionStore := cachedRegion.getStore()

tikvCnt := 0
for i, store := range regionStore.stores {

This comment has been minimized.

Copy link
@lysu

lysu Sep 24, 2019

Member

Do we have the situation that 1 region have multiple TiFlash?

if so we should take care failure in one of TiFlash store and loadbalance between multiple TiFlash node.

for example, region1 has 3 TiFlash node: a, b, c.

we should give chance to use b and c; and send request to b and c when a is failured?

@lysu lysu added status/LGT2 and removed status/LGT1 labels Sep 24, 2019
Copy link
Member

left a comment

LGTM

store/tikv/region_cache.go Outdated Show resolved Hide resolved
store/tikv/region_cache.go Outdated Show resolved Hide resolved
store/tikv/region_cache.go Outdated Show resolved Hide resolved
@lzmhhh123 lzmhhh123 requested a review from lysu Sep 25, 2019
@lysu
lysu approved these changes Sep 25, 2019
Copy link
Member

left a comment

LGTM

@sre-bot

This comment has been minimized.

Copy link

commented Sep 25, 2019

/run-all-tests

@sre-bot sre-bot merged commit b25b703 into pingcap:master Sep 25, 2019
13 checks passed
13 checks passed
idc-jenkins-ci-tidb/build Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/build_check_race Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/check_dev Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/check_dev_2 Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/common-test job succeeded
Details
idc-jenkins-ci-tidb/integration-common-test Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/integration-compatibility-test Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/integration-ddl-test Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/mybatis-test job succeeded
Details
idc-jenkins-ci-tidb/sqllogic-test-1 Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/sqllogic-test-2 Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/unit-test Jenkins job succeeded.
Details
license/cla Contributor License Agreement is signed.
Details
@lzmhhh123 lzmhhh123 deleted the lzmhhh123:dev/add_theflash_access_flag branch Sep 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.