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

infoschema: fix tidb version showed in cluster_info table and update the go.mod for new sysutil #16003

Merged
merged 27 commits into from
Apr 9, 2020
Merged

Conversation

reafans
Copy link
Contributor

@reafans reafans commented Apr 2, 2020

What problem does this PR solve?

The version of TiDB is not consistent with TiKV and PD.

What is changed and how it works?

Proposal: xxx

What's Changed:
before:

+------+-----------------+-----------------+----------------------------------------+------------------------------------------+---------------------------+--------------------+
| TYPE | INSTANCE        | STATUS_ADDRESS  | VERSION                                | GIT_HASH                                 | START_TIME
          | UPTIME             |
+------+-----------------+-----------------+----------------------------------------+------------------------------------------+---------------------------+--------------------+
| tidb | 0.0.0.0:4000    | 0.0.0.0:10080   | 5.7.25-TiDB-v4.0.0-beta-613-gaa7ef9c46 | aa7ef9c46435bb20d42fa0badcf09fc16f9609a9 | 2020-04-02T15:35:56+08:00 | 8.410056522s       |
| pd   | 127.0.0.1:2379  | 127.0.0.1:2379  | 4.0.0-beta.2                           | 7ebad5232e7df681d8d425900f52984574539568 | 2020-04-02T10:15:36+08:00 | 5h20m28.410060208s |
| tikv | 127.0.0.1:20160 | 127.0.0.1:20180 | 4.0.0-beta.2                           | 7908f6e6699239fff23daa444961b5a47ff659da | 2020-04-02T10:15:49+08:00 | 5h20m15.410061844s |
+------+-----------------+-----------------+----------------------------------------+------------------------------------------+---------------------------+--------------------+

this pr : not set the serverVersion, use select Version() would get the default version. And CLUSTER_INFO would get the format version.

mysql root@localhost:INFORMATION_SCHEMA> select Version()
+----------------------------------------------+
| Version()                                    |
+----------------------------------------------+
| 5.7.25-TiDB-v4.0.0-beta-630-g91effa93d-dirty |
+----------------------------------------------+
1 row in set
Time: 0.030s

mysql root@localhost:INFORMATION_SCHEMA> select * from `CLUSTER_INFO`
+------+-----------------+-----------------+--------------+------------------------------------------+---------------------------+--------------------+
| TYPE | INSTANCE        | STATUS_ADDRESS  | VERSION      | GIT_HASH                                 | START_TIME                | UPTIME             |
+------+-----------------+-----------------+--------------+------------------------------------------+---------------------------+--------------------+
| tidb | 0.0.0.0:4000    | 0.0.0.0:10080   | 4.0.0-beta   | 91effa93d86cf0bb7545e9d59f6a1767b1bfdce5 | 2020-04-07T21:21:18+08:00 | 1m39.481884397s    |
| pd   | 127.0.0.1:2379  | 127.0.0.1:2379  | 4.0.0-beta.2 | 7ebad5232e7df681d8d425900f52984574539568 | 2020-04-07T11:59:11+08:00 | 9h23m46.481888567s |
| tikv | 127.0.0.1:20160 | 127.0.0.1:20180 | 4.0.0-beta.2 | 7908f6e6699239fff23daa444961b5a47ff659da | 2020-04-07T11:59:31+08:00 | 9h23m26.48189004s  |
+------+-----------------+-----------------+--------------+------------------------------------------+---------------------------+--------------------+
3 rows in set
Time: 0.033s

When set the serverVersion to "8.0.1" in config.toml, select Version() and CLUSTER_INFO would get the version which user set:

mysql root@localhost:INFORMATION_SCHEMA> select Version()
+-----------+
| Version() |
+-----------+
| 8.0.1     |
+-----------+
1 row in set
Time: 0.033s

mysql root@localhost:INFORMATION_SCHEMA> select * from `CLUSTER_INFO`
+------+-----------------+-----------------+--------------+------------------------------------------+---------------------------+--------------------+
| TYPE | INSTANCE        | STATUS_ADDRESS  | VERSION      | GIT_HASH                                 | START_TIME                | UPTIME             |
+------+-----------------+-----------------+--------------+------------------------------------------+---------------------------+--------------------+
| tidb | 0.0.0.0:4000    | 0.0.0.0:10080   | 8.0.1        | None                                     | 2020-04-07T21:25:55+08:00 | 59.239367762s      |
| pd   | 127.0.0.1:2379  | 127.0.0.1:2379  | 4.0.0-beta.2 | 7ebad5232e7df681d8d425900f52984574539568 | 2020-04-07T11:59:11+08:00 | 9h27m43.239373343s |
| tikv | 127.0.0.1:20160 | 127.0.0.1:20180 | 4.0.0-beta.2 | 7908f6e6699239fff23daa444961b5a47ff659da | 2020-04-07T11:59:31+08:00 | 9h27m23.239374837s |
+------+-----------------+-----------------+--------------+------------------------------------------+---------------------------+--------------------+
3 rows in set

How it Works:

Check List

Tests

  • Unit test

@reafans reafans requested a review from a team as a code owner April 2, 2020 05:51
@CLAassistant
Copy link

CLAassistant commented Apr 2, 2020

CLA assistant check
All committers have signed the CLA.

@ghost ghost requested review from qw4990 and removed request for a team April 2, 2020 05:51
@github-actions github-actions bot added sig/sql-infra SIG: SQL Infra sig/execution SIG execution labels Apr 2, 2020
@reafans
Copy link
Contributor Author

reafans commented Apr 2, 2020

/run-unit-test

@reafans reafans changed the title infoschema: fix tidb version showed in cluster_info table. infoschema: fix tidb version showed in cluster_info table and update the go.mod for new sysutil Apr 2, 2020
@codecov
Copy link

codecov bot commented Apr 2, 2020

Codecov Report

Merging #16003 into master will not change coverage by %.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master     #16003   +/-   ##
===========================================
  Coverage   80.3136%   80.3136%           
===========================================
  Files           506        506           
  Lines        136226     136226           
===========================================
  Hits         109408     109408           
  Misses        18301      18301           
  Partials       8517       8517           

Copy link
Contributor

@Deardrops Deardrops left a comment

Choose a reason for hiding this comment

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

LGTM

@Deardrops Deardrops added the status/LGT1 Indicates that a PR has LGTM 1. label Apr 7, 2020
// FormatVersion make TiDBVersion consistent to TiKV and PD.
func FormatVersion(TiDBVersion string) string {
var version string
nodeVersion := TiDBVersion[strings.LastIndex(TiDBVersion, "TiDB-")+len("TiDB-"):]
Copy link
Contributor

Choose a reason for hiding this comment

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

Check len(TiDBVersion) > n before use

Copy link
Contributor

Choose a reason for hiding this comment

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

+1.

Since the TiDBVersion maybe equal the config ServerVersion, Add more case for this, such as xxx

Copy link
Contributor Author

@reafans reafans Apr 7, 2020

Choose a reason for hiding this comment

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

Test it need change the serverVersion config, so test it locally and show the result in introduction.

  1. When not set the serverVersion, it would use the default version, and format it in cluster_info.
  2. When the user set the serverVersion(this would hardly happend), it would use the serverVersion and not do the format.

@Deardrops Deardrops removed the status/LGT1 Indicates that a PR has LGTM 1. label Apr 7, 2020
@reafans
Copy link
Contributor Author

reafans commented Apr 9, 2020

/run-check_dev_2

Copy link
Contributor

@zimulala zimulala left a comment

Choose a reason for hiding this comment

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

LGTM

@zimulala zimulala added the status/can-merge Indicates a PR has been approved by a committer. label Apr 9, 2020
@sre-bot
Copy link
Contributor

sre-bot commented Apr 9, 2020

Your auto merge job has been accepted, waiting for 16219, 16138

@zimulala zimulala added the status/LGT2 Indicates that a PR has LGTM 2. label Apr 9, 2020

// The user hasn't set the config 'ServerVersion'.
if isDefaultVersion {
nodeVersion = TiDBVersion[strings.LastIndex(TiDBVersion, "TiDB-")+len("TiDB-"):]
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a scenario where "TiDBVersion" is not the default format, but "isDefaultVersion" is true?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No,this would be never happened,because the "TiDBVersion" also comes from mysql.ServerVersion, and when the user not set the serverVersion,the mysql.ServerVersion would be the default version which make isDefaultVersion become true.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK. I see node.Version is come from ServerInfo so ask this question.

@sre-bot
Copy link
Contributor

sre-bot commented Apr 9, 2020

/run-all-tests

@reafans
Copy link
Contributor Author

reafans commented Apr 9, 2020

/merge

@sre-bot
Copy link
Contributor

sre-bot commented Apr 9, 2020

Your auto merge job has been accepted, waiting for 16142, 16139, 16137, 16204

@sre-bot
Copy link
Contributor

sre-bot commented Apr 9, 2020

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Apr 9, 2020

@reafans merge failed.

@reafans
Copy link
Contributor Author

reafans commented Apr 9, 2020

/run-integration-copr-test

@reafans
Copy link
Contributor Author

reafans commented Apr 9, 2020

/run-integration-copr-test tikv=pr/7383

@reafans
Copy link
Contributor Author

reafans commented Apr 9, 2020

/run-check_dev_2

@reafans
Copy link
Contributor Author

reafans commented Apr 9, 2020

/merge

@sre-bot
Copy link
Contributor

sre-bot commented Apr 9, 2020

/run-all-tests

@sre-bot sre-bot merged commit c635ac8 into pingcap:master Apr 9, 2020
@reafans reafans deleted the fix_tidb_version branch April 9, 2020 05:43
ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Jul 7, 2020
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot
Copy link
Contributor

cherry pick to release-4.0 in PR #18413

ti-srebot added a commit that referenced this pull request Jul 28, 2020
…the go.mod for new sysutil (#16003) (#18413)

Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
Signed-off-by: crazycs520 <crazycs520@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sig/execution SIG execution sig/sql-infra SIG: SQL Infra status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants