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

server: fix panic in api regions/meta #10222

Merged
merged 4 commits into from May 9, 2019

Conversation

Projects
None yet
7 participants
@winkyao
Copy link
Member

commented Apr 22, 2019

What problem does this PR solve?

func (t *tikvHandlerTool) getRegionsMeta(regionIDs []uint64) ([]RegionMeta, error) {
	regions := make([]RegionMeta, len(regionIDs))
	for i, regionID := range regionIDs {
		meta, leader, err := t.RegionCache.PDClient().GetRegionByID(context.TODO(), regionID)
		if err != nil {
			return nil, errors.Trace(err)
		}

		regions[i] = RegionMeta{
			ID:          regionID,
			Leader:      leader,
			Peers:       meta.Peers,
			RegionEpoch: meta.RegionEpoch,
		}

	}
	return regions, nil
}

if t.RegionCache.PDClient().GetRegionByID(context.TODO(), regionID) return nil, nil, nil. panic will throw in meta.Peers. when the tidb just start, the region cache can be empty, so the return value of GetRegionByID can be nil.

Check List

Tests

  • Unit test

Side effects

  • Increased code complexity
@codecov

This comment has been minimized.

Copy link

commented Apr 22, 2019

Codecov Report

Merging #10222 into master will increase coverage by 0.0083%.
The diff coverage is 100%.

@@               Coverage Diff                @@
##             master     #10222        +/-   ##
================================================
+ Coverage   77.3224%   77.3307%   +0.0083%     
================================================
  Files           412        412                
  Lines         85847      85852         +5     
================================================
+ Hits          66379      66390        +11     
+ Misses        14417      14412         -5     
+ Partials       5051       5050         -1
@lamxTyler
Copy link
Member

left a comment

LGTM


failpoint.Inject("errGetRegionByIDEmpty", func(val failpoint.Value) {
if val.(bool) {
meta = nil

This comment has been minimized.

Copy link
@jackysp

jackysp Apr 22, 2019

Member

Why not return directly?

This comment has been minimized.

Copy link
@winkyao

winkyao Apr 22, 2019

Author Member

I wanna inject the case that meta is nil.

This comment has been minimized.

Copy link
@jackysp

jackysp Apr 22, 2019

Member

Is it better to inject it in GetRegionByID?

This comment has been minimized.

Copy link
@crazycs520

crazycs520 Apr 23, 2019

Contributor

@jackysp GetRegionByID function is in PD, How to inject?

This comment has been minimized.

Copy link
@jackysp

jackysp Apr 23, 2019

Member

I think there is another implement in mocktikv.

@jackysp
Copy link
Member

left a comment

LGTM

@jackysp

This comment has been minimized.

Copy link
Member

commented Apr 23, 2019

/run-all-tests

@zz-jason zz-jason added status/LGT2 and removed status/LGT1 labels Apr 23, 2019

@zimulala

This comment has been minimized.

Copy link
Member

commented Apr 30, 2019

Please fix the CI.

ngaut added some commits May 8, 2019

@ngaut ngaut merged commit 990e256 into pingcap:master May 9, 2019

8 checks passed

ci/circleci Your tests passed on CircleCI!
Details
codecov/patch 100% of diff hit (target 0%)
Details
codecov/project 77.3307% (+0.0083%) compared to c2e83af
Details
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
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.