Skip to content
This repository has been archived by the owner on Dec 8, 2021. It is now read-only.

Check if the cluster satisfies the version requirements before starting #61

Merged
merged 7 commits into from
Sep 7, 2018
Merged

Check if the cluster satisfies the version requirements before starting #61

merged 7 commits into from
Sep 7, 2018

Conversation

kennytm
Copy link
Collaborator

@kennytm kennytm commented Sep 2, 2018

Version check involving tikv-importer isn't included yet.

In the future we could add more sanity checks from the checklist (e.g. table-concurrency should be ≤ tikv-importer's max-open-engines).

@sre-bot

This comment has been minimized.

Sign in to view
@kennytm

This comment has been minimized.

Sign in to view
@kennytm
Copy link
Collaborator Author

kennytm commented Sep 2, 2018

/run-all-tests

@kennytm
Copy link
Collaborator Author

kennytm commented Sep 2, 2018

/run-integration-test

I suspect the Jenkins failure is caused by the close/open gRPC messages being reordered (e.g. open1 → open2 → close1 → open3 got received as open1 → open2 → open3 → close1 and the max-open-engine is violated at the 3rd step), but I can't repro it anymore.

@kennytm
Copy link
Collaborator Author

kennytm commented Sep 3, 2018

@holys PTAL

holys
holys reviewed Sep 3, 2018
View reviewed changes
lightning/restore/restore.go
@@ -40,6 +43,12 @@ const (
closeEngineMaxRetry = 5
)

var (
requiredTiDBVersion = *semver.New("2.0.4")
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not use pointer?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@holys Their Compare function doesn't use pointers :(

holys
holys reviewed Sep 3, 2018
View reviewed changes
lightning/common/util.go Outdated
@@ -210,3 +213,21 @@ func isRetryableError(err error) bool {
func UniqueTable(schema string, table string) string {
return fmt.Sprintf("`%s`.`%s`", schema, table)
}

func GetJSON(client *http.Client, url string, v interface{}) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please comment that v must be a pointer type.

@holys
Copy link
Contributor

holys commented Sep 3, 2018

LGTM Good job!

@holys
Copy link
Contributor

holys commented Sep 3, 2018

@csuzhangxc PTAL

@kennytm
Copy link
Collaborator Author

kennytm commented Sep 4, 2018

Turns out the PD and TiKV version reporting is added quite recently in the 2.1 branch (tikv/pd#1148, pingcap/kvproto#285, tikv/tikv#3319). This means if we want to target the 2.0 branch we cannot check for PD and TiKV's versions.

@holys
Copy link
Contributor

holys commented Sep 5, 2018

@kennytm Should we do something when there is no version information in PD and TiKV?

@kennytm
Copy link
Collaborator Author

kennytm commented Sep 5, 2018

@holys I'd like to just comment out the PD and TiKV check until 2.1 is stabilized, and then bump the requirements. Or we could find a feature which is only present on 2.0.4+ and do feature detection.

@kennytm kennytm force-pushed the kennytm/check-min-req branch from 25cfd1a to f31ef92 Compare September 5, 2018 03:17
@kennytm
Copy link
Collaborator Author

kennytm commented Sep 5, 2018

/run-integration-test

@holys @csuzhangxc PTAL again

@holys
Copy link
Contributor

holys commented Sep 5, 2018

LGTM again

@kennytm
vendor: insert go-semver dependecy
2a75b1c
@kennytm
tidb, util: extract the HTTP JSON fetching code into its own function
b7d1c04
@kennytm
*: check for cluster versions before starting
c9478db
@kennytm
restore_test: added logs to detect spurious failure
c2f9be1 
Also, make the mock import operation only wait while we're still opening
engines to speed up the test.
@kennytm
util: document GetJSON()
47029f3
@kennytm
restore: use a more robust version parsing strategy for TiDB
e01819d 
In semver, '2.0.4-1 < 2.0.4', so we need to properly skip the unrelated
portions of the version.
@kennytm
restore, kv: detect if SwitchMode exists instead of check version
f800ab4 
TiKV and PD's versions are exposed before 2.1, so we can't rely on their
APIs to verify for 2.0.4+. Since the only reason we need 2.0.4+ is the
SwitchMode gRPC interface, we instead inspect if SwitchMode is
unimplemented and display a user-friendly message on stderr.
@kennytm kennytm force-pushed the kennytm/check-min-req branch from f31ef92 to f800ab4 Compare September 6, 2018 06:34
@kennytm
Copy link
Collaborator Author

kennytm commented Sep 6, 2018

/run-all-tests

(just rebased on top of master, nothing is changed)

csuzhangxc
csuzhangxc approved these changes Sep 7, 2018
View reviewed changes
Copy link
Member

@csuzhangxc csuzhangxc left a comment

Choose a reason for hiding this comment

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

LGTM

@kennytm kennytm merged commit 6714e28 into master Sep 7, 2018
@kennytm kennytm deleted the kennytm/check-min-req branch September 7, 2018 09:16
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@holys holys holys left review comments

@csuzhangxc csuzhangxc csuzhangxc approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kennytm @sre-bot @holys @csuzhangxc