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.

Sign up for GitHub

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

Jump to bottom

domain: report min start timestamp of TiDB server #12133

Merged
merged 8 commits into from
Sep 17, 2019
Merged

domain: report min start timestamp of TiDB server #12133

merged 8 commits into from
Sep 17, 2019

Conversation

jackysp
Copy link
Member

@jackysp jackysp commented Sep 10, 2019

What problem does this PR solve?

The server current min start timestamp could be reported to PD, it may be used to calculate a more reasonable safe point for GC.

What is changed and how it works?

Report min start timestamp to PD.

Check List

Tests

  • Manual test (add detailed scripts or steps below)
  1. start a TiDB cluster.
  2. ETCDCTL_API=3 ./etcdctl --endpoints="http://172.16.5.34:2380" get / --prefix gets the key /tidb/server/minstartts/xxx
  3. start a connection and begin a transaction, then get the keys from PD again.
  4. commit the transaction, then get the keys from PD again.

Code changes

  • Has exported function/method change

@jackysp
domain: report min start timestamp for TiDB server
d6b708b 
Signed-off-by: Shuaipeng Yu <jackysp@gmail.com>
@jackysp
add debug logs
bec9246 
Signed-off-by: Shuaipeng Yu <jackysp@gmail.com>
@jackysp
set session manager for it
4269a29 
Signed-off-by: Shuaipeng Yu <jackysp@gmail.com>
@jackysp
remove debug logs
bcf9859 
Signed-off-by: Shuaipeng Yu <jackysp@gmail.com>
@jackysp
fix lint
fbc6a84 
Signed-off-by: Shuaipeng Yu <jackysp@gmail.com>
@jackysp jackysp added the type/enhancement The issue or PR belongs to an enhancement. label Sep 10, 2019
@codecov
Copy link

codecov bot commented Sep 10, 2019 •
edited
Loading

Codecov Report

Merging #12133 into master will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##            master    #12133   +/-   ##
=========================================
  Coverage   81.428%   81.428%           
=========================================
  Files          454       454           
  Lines       100000    100000           
=========================================
  Hits         81428     81428           
  Misses       12805     12805           
  Partials      5767      5767

imtbkcat
imtbkcat reviewed Sep 10, 2019
View reviewed changes
Copy link

@imtbkcat imtbkcat left a comment

Choose a reason for hiding this comment

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

LGTM

@jackysp jackysp requested a review from disksing September 11, 2019 03:20
MyonKeminta
MyonKeminta reviewed Sep 11, 2019
View reviewed changes
domain/info.go
}
return util.PutKVToEtcd(ctx, is.etcdCli, keyOpDefaultRetryCnt, is.minStartTSPath,
strconv.FormatUint(is.minStartTS, 10),
clientv3.WithLease(is.session.Lease()))
Copy link
Contributor

Choose a reason for hiding this comment

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

Does all TiDB servers updates the same key in etcd? If so, how do TiDB servers know whether there's another TiDB server who has a smaller minStartTs?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I got it. LGTM.

coocood
coocood reviewed Sep 11, 2019
View reviewed changes
domain/info.go
}
pl := is.manager.ShowProcessList()
var minStartTS uint64 = math.MaxUint64
for _, info := range pl {
Copy link
Member

Choose a reason for hiding this comment

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

What if len(pl) == 0?

Copy link
Member Author

Choose a reason for hiding this comment

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

Report max uint64 to PD.

@coocood
Copy link
Member

coocood commented Sep 11, 2019

If a tidb-server is killed -9, how long will the minStartTS be kept in PD?

@jackysp
Copy link
Member Author

jackysp commented Sep 11, 2019

If a tidb-server is killed -9, how long will the minStartTS be kept in PD?

1 min. It used the same TTL as server info,

tidb/domain/info.go

Line 221 in fbc6a84

session, err := owner.NewSession(ctx, logPrefix, is.etcdCli, retryCnt, InfoSessionTTL)

@jackysp
do not report when server starts
6e159ca 
Signed-off-by: Shuaipeng Yu <jackysp@gmail.com>
@jackysp
Copy link
Member Author

jackysp commented Sep 14, 2019

PTAL @coocood

@coocood
Copy link
Member

coocood commented Sep 16, 2019

LGTM

@jackysp jackysp requested a review from zimulala September 16, 2019 07:34
lysu
lysu reviewed Sep 16, 2019
View reviewed changes
Copy link
Contributor

@lysu lysu left a comment

Choose a reason for hiding this comment

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

LGTM :shipit:

zimulala
zimulala reviewed Sep 16, 2019
View reviewed changes
domain/info.go
@@ -49,6 +54,9 @@ type InfoSyncer struct {
etcdCli *clientv3.Client
info *ServerInfo
serverInfoPath string
minStartTS uint64
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems that we don't need this field.

Copy link
Contributor

Choose a reason for hiding this comment

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

It will be used by the next PR.

zimulala
zimulala approved these changes Sep 16, 2019
View reviewed changes
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 Sep 16, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Sep 16, 2019

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Sep 16, 2019

@jackysp merge failed.

@jackysp
Copy link
Member Author

jackysp commented Sep 17, 2019

/merge

@sre-bot
Copy link
Contributor

sre-bot commented Sep 17, 2019

/run-all-tests

@sre-bot sre-bot merged commit bc4f8ce into pingcap:master Sep 17, 2019
@jackysp jackysp deleted the report_min_startts branch February 27, 2020 13:34
@tangenta tangenta mentioned this pull request Apr 13, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@imtbkcat imtbkcat imtbkcat left review comments

@MyonKeminta MyonKeminta MyonKeminta left review comments

@zimulala zimulala zimulala approved these changes

@coocood coocood coocood left review comments

@lysu lysu lysu left review comments

@disksing disksing Awaiting requested review from disksing

Assignees
No one assigned
Labels
status/can-merge Indicates a PR has been approved by a committer. type/enhancement The issue or PR belongs to an enhancement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@jackysp @coocood @sre-bot @lysu @zimulala @MyonKeminta @imtbkcat