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

Configure default pd endpoint for tikv-server #5427

Merged
merged 9 commits into from Sep 16, 2019

Conversation

@aknuds1
Copy link
Contributor

aknuds1 commented Sep 9, 2019

What have you changed?

Add sensible default for --pd-endpoints flag ("127.0.0.1:2379"), so that one can easily run tikv-server for local testing.

What is the type of the changes?

  • Improvement (a change which is an improvement to an existing feature)

How is the PR tested?

I tested manually and with make test.

Does this PR affect documentation (docs) or should it be mentioned in the release notes?

Not sure.

Does this PR affect tidb-ansible?

No.

@aknuds1 aknuds1 force-pushed the aknuds1:feature/default-pd-endpoints branch from 314d25f to c2961f4 Sep 9, 2019
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
@aknuds1 aknuds1 force-pushed the aknuds1:feature/default-pd-endpoints branch from c2961f4 to e874a81 Sep 9, 2019
Copy link
Contributor

overvenus left a comment

👍

@sre-bot

This comment has been minimized.

Copy link
Collaborator

sre-bot commented Sep 9, 2019

Your auto merge job has been accepted, waiting for #5374

@sre-bot

This comment has been minimized.

Copy link
Collaborator

sre-bot commented Sep 9, 2019

/run-all-tests

@sre-bot

This comment has been minimized.

Copy link
Collaborator

sre-bot commented Sep 9, 2019

@aknuds1 merge failed.

@aknuds1 aknuds1 changed the title Feature/default pd endpoints Configure default pd endpoint for tikv-server Sep 9, 2019
@aknuds1

This comment has been minimized.

Copy link
Contributor Author

aknuds1 commented Sep 9, 2019

@siddontang Do you know why the integration test fails?

@breeswish

This comment has been minimized.

Copy link
Member

breeswish commented Sep 10, 2019

/merge

@breeswish

This comment has been minimized.

Copy link
Member

breeswish commented Sep 10, 2019

@aknuds1 Don't worry, it's simply unstable tests.

@sre-bot

This comment has been minimized.

Copy link
Collaborator

sre-bot commented Sep 10, 2019

/run-all-tests

@sre-bot

This comment has been minimized.

Copy link
Collaborator

sre-bot commented Sep 10, 2019

@aknuds1 merge failed.

@breeswish

This comment has been minimized.

Copy link
Member

breeswish commented Sep 10, 2019

/run-integration-cop-push-down-test

1 similar comment
@you06

This comment has been minimized.

Copy link
Contributor

you06 commented Sep 10, 2019

/run-integration-cop-push-down-test

@you06

This comment has been minimized.

Copy link
Contributor

you06 commented Sep 10, 2019

/run-integration-cop-push-down-test

@H-ZeX

This comment has been minimized.

Copy link
Contributor

H-ZeX commented Sep 11, 2019

if run the pd, tikv, tidb, with this config

#  tikv's config

log-level = "info"

[server]
# end-point-enable-batch-if-possible = false
## Listening address.
# addr = "127.0.0.1:20161"
# status-addr = ""

[pd]
endpoints = ["127.0.0.1:3379"]

[storage]
data-dir = "/tmp/tikv_no_batch"
#  pd's config

name = "pd"
data-dir = "/tmp/pd_no_batch"
client-urls = "http://127.0.0.1:3379"
# peer-urls = "http://127.0.0.1:3380"

[log]
level = "info"
# tidb's config

port = 4006
store = "tikv"
path = "127.0.0.1:3379"

[log]
level = "info"

# [status]
# report-status = false

according to tikv's log ["connecting to PD endpoint"] [endpoints=127.0.0.1:2379], although if we use command line param, it is ok.
thanks for @you06 found the reason

@zhouqiang-cl

This comment has been minimized.

Copy link
Member

zhouqiang-cl commented Sep 11, 2019

So. It will bring default config file problem. I think the priority should be command config > config file> code config.
@aknuds1 How did you think about it

@aknuds1

This comment has been minimized.

Copy link
Contributor Author

aknuds1 commented Sep 11, 2019

@zhouqiang-cl I wasn't aware that the default would take precedence over config file. I will see if I can figure out how to fix this.

@breeswish

This comment has been minimized.

Copy link
Member

breeswish commented Sep 11, 2019

So maybe you can consider modifying the default value for PD endpoint config instead of PD endpoint command line args?

@H-ZeX

This comment has been minimized.

Copy link
Contributor

H-ZeX commented Sep 11, 2019

I wasn't aware that the default would take precedence over command line. I will see if I can figure out how to fix this.

@aknuds1
The default value set by code doesn't take precedence over command line, but it take precedence over config file. This is wrong.

@aknuds1

This comment has been minimized.

Copy link
Contributor Author

aknuds1 commented Sep 11, 2019

@H-ZeX Sorry, I meant to say I wasn't aware the command line default took precedence over config file. I'm trying now to change the config default instead of the command line default as @breeswish suggested.

Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
@aknuds1 aknuds1 dismissed stale reviews from siddontang and overvenus via 402d1ed Sep 11, 2019
@aknuds1

This comment has been minimized.

Copy link
Contributor Author

aknuds1 commented Sep 11, 2019

@breeswish @H-ZeX @zhouqiang-cl I pushed a change where the default is defined for TiKvConfig instead of for the command line parser. Can you please provide feedback on whether this is the correct approach? It seems to work.

@breeswish

This comment has been minimized.

Copy link
Member

breeswish commented Sep 11, 2019

/run-all-tests

@zhouqiang-cl

This comment has been minimized.

Copy link
Member

zhouqiang-cl commented Sep 11, 2019

/test

@breeswish

This comment has been minimized.

Copy link
Member

breeswish commented Sep 12, 2019

/run-all-tests

@breeswish

This comment has been minimized.

Copy link
Member

breeswish commented Sep 12, 2019

According to CI, config::tests::test_pd_cfg failed and you need to fix it.

@aknuds1

This comment has been minimized.

Copy link
Contributor Author

aknuds1 commented Sep 12, 2019

@breeswish ok, will do.

aknuds1 added 3 commits Sep 12, 2019
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
…v into feature/default-pd-endpoints
@aknuds1

This comment has been minimized.

Copy link
Contributor Author

aknuds1 commented Sep 12, 2019

@breeswish I fixed the test.

@zhouqiang-cl

This comment has been minimized.

Copy link
Member

zhouqiang-cl commented Sep 14, 2019

/test

Copy link
Contributor

siddontang left a comment

LGTM

Btw, seem we can show our default values in command help output. :-)

@aknuds1

This comment has been minimized.

Copy link
Contributor Author

aknuds1 commented Sep 14, 2019

LGTM

Btw, seem we can show our default values in command help output. :-)

Yeah, I was actually wondering if I should add the default value to the command line help. I can do it in another PR?

@breeswish

This comment has been minimized.

Copy link
Member

breeswish commented Sep 15, 2019

/merge

@sre-bot

This comment has been minimized.

Copy link
Collaborator

sre-bot commented Sep 15, 2019

/run-all-tests

@sre-bot

This comment has been minimized.

Copy link
Collaborator

sre-bot commented Sep 15, 2019

@aknuds1 merge failed.

@breeswish

This comment has been minimized.

Copy link
Member

breeswish commented Sep 16, 2019

/merge

@sre-bot

This comment has been minimized.

Copy link
Collaborator

sre-bot commented Sep 16, 2019

/run-all-tests

@sre-bot sre-bot merged commit 385d4b4 into tikv:master Sep 16, 2019
6 checks passed
6 checks passed
DCO All commits are signed off!
Details
idc-jenkins-ci-tikv/integration-common-test Jenkins job succeeded.
Details
idc-jenkins-ci-tikv/integration-compatibility-test Jenkins job succeeded.
Details
idc-jenkins-ci-tikv/integration-cop-push-down-test Jenkins job succeeded.
Details
idc-jenkins-ci-tikv/integration-ddl-test Jenkins job succeeded.
Details
idc-jenkins-ci/test Jenkins job succeeded.
Details
@aknuds1 aknuds1 deleted the aknuds1:feature/default-pd-endpoints branch Sep 25, 2019
sticnarf pushed a commit to sticnarf/tikv that referenced this pull request Oct 27, 2019
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.