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 caused by GetLeader #1766

Merged
merged 5 commits into from Sep 25, 2019

Conversation

@rleungx
Copy link
Member

commented Sep 24, 2019

What problem does this PR solve?

Before executing startEtcd, if we send an HTTP request, e.g. curl http://127.0.0.1:2379/pd/api/v1/version. It will panic due to the nil pointer problem. See #1764.

What is changed and how it works?

This PR fixes this problem by using an empty struct.

Check List

Tests

  • Unit test

Related changes

  • Need to cherry-pick to the release branch
  • Need to be included in the release notes
Signed-off-by: Ryan Leung <rleungx@gmail.com>
@codecov-io

This comment has been minimized.

Copy link

commented Sep 24, 2019

Codecov Report

Merging #1766 into master will decrease coverage by 0.18%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1766      +/-   ##
==========================================
- Coverage   77.45%   77.27%   -0.19%     
==========================================
  Files         163      163              
  Lines       16450    16454       +4     
==========================================
- Hits        12742    12715      -27     
- Misses       2663     2683      +20     
- Partials     1045     1056      +11
Impacted Files Coverage Δ
server/server.go 82.35% <100%> (-0.44%) ⬇️
server/region_syncer/client.go 68.42% <0%> (-14.48%) ⬇️
pkg/etcdutil/etcdutil.go 76.81% <0%> (-11.6%) ⬇️
pkg/metricutil/metricutil.go 90.62% <0%> (-9.38%) ⬇️
server/tso/tso.go 77.06% <0%> (-6.43%) ⬇️
server/checker/replica_checker.go 78.82% <0%> (-1.77%) ⬇️
server/grpc_service.go 57.04% <0%> (-1.74%) ⬇️
server/member/leader.go 76.53% <0%> (ø) ⬆️
server/cluster.go 84.54% <0%> (ø) ⬆️
... and 2 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 3017268...fea171f. Read the comment docs.

Copy link
Member

left a comment

rest LGTM

server/api/version_test.go Show resolved Hide resolved
rleungx added 2 commits Sep 24, 2019
Signed-off-by: Ryan Leung <rleungx@gmail.com>
Signed-off-by: Ryan Leung <rleungx@gmail.com>
@rleungx rleungx force-pushed the rleungx:fix-panic branch from d973c43 to d3e5fd7 Sep 24, 2019
@nolouch nolouch added the CanMerge label Sep 25, 2019
@sre-bot

This comment has been minimized.

Copy link

commented Sep 25, 2019

/run-all-tests

@sre-bot

This comment has been minimized.

Copy link

commented Sep 25, 2019

@rleungx merge failed.

@rleungx rleungx force-pushed the rleungx:fix-panic branch from 1372dce to 44c0dce Sep 25, 2019
Signed-off-by: Ryan Leung <rleungx@gmail.com>
@rleungx rleungx force-pushed the rleungx:fix-panic branch from 38351da to 36c2df2 Sep 25, 2019
@rleungx

This comment has been minimized.

Copy link
Member Author

commented Sep 25, 2019

/merge

@sre-bot

This comment has been minimized.

Copy link

commented Sep 25, 2019

/run-all-tests

@sre-bot sre-bot merged commit e2dbdf6 into pingcap:master Sep 25, 2019
8 checks passed
8 checks passed
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
idc-jenkins-ci-pd/integration-common-test Jenkins job succeeded.
Details
idc-jenkins-ci-pd/integration-compatibility-test Jenkins job succeeded.
Details
idc-jenkins-ci-pd/integration-ddl-test Jenkins job succeeded.
Details
idc-jenkins-ci/build Jenkins job succeeded.
Details
idc-jenkins-ci/test Jenkins job succeeded.
Details
license/cla Contributor License Agreement is signed.
Details
@sre-bot

This comment has been minimized.

Copy link

commented Sep 25, 2019

cherry pick to release-3.1 failed

@rleungx rleungx deleted the rleungx:fix-panic branch Oct 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.