server: use leader lease to determine tso service validity #1676

disksing · 2019-08-09T11:33:54Z

STATUS: It seems to be better merged after #1668. You can review it now and give comments.

What problem does this PR solve?

Currently, the safety of timestamp allocation relies on the leader flag be updated in time when lease timeout.
There is a small possibility (for instance, process paused, runtime schedule slow) that the watch channel does not notify in time, which may cause 2 PD servers serve timestamp at the same time. Then the transaction may become corrupted.

Another issue is when a PD server becomes leader, it won't serve timestamps until the leader flag is set. But the leader flag is set after loading all regions from storage, which may take considerably long time for a large cluster.

This PR fixes #1661 and part of #1658

What is changed and how it works?

Introduce LeaderLease to periodically renew lease and update expire time.

Timestamp service only relies on the lease expire time.

Note that if 2 time.Times both have monotonic part, comparison of them will be evaluated by monotonic time, which is not affected by wall clock changes.

Check List

Tests

Unit test

Side effects

Increased code complexity
May introduce bugs

Related changes

Need to be included in the release notes

Signed-off-by: disksing <i@disksing.com>

codecov-io · 2019-08-09T11:43:51Z

Codecov Report

Merging #1676 into master will decrease coverage by 0.03%.
The diff coverage is 89.02%.

@@            Coverage Diff             @@
##           master    #1676      +/-   ##
==========================================
- Coverage    76.3%   76.26%   -0.04%     
==========================================
  Files         157      158       +1     
  Lines       15347    15402      +55     
==========================================
+ Hits        11710    11747      +37     
- Misses       2644     2654      +10     
- Partials      993     1001       +8

Impacted Files	Coverage Δ
server/grpc_service.go	`58.87% <0%> (+0.43%)`	⬆️
server/tso/tso.go	`83.48% <50%> (+4.24%)`	⬆️
server/leader.go	`79.14% <87.5%> (-3.16%)`	⬇️
server/lease.go	`95% <95%> (ø)`
pkg/etcdutil/etcdutil.go	`76.81% <0%> (-5.8%)`	⬇️
server/kv/etcd_kv.go	`79.22% <0%> (-2.6%)`	⬇️
client/client.go	`65.7% <0%> (-2.48%)`	⬇️
server/core/storage.go	`76.08% <0%> (-1.45%)`	⬇️
server/cluster.go	`83.72% <0%> (-0.24%)`	⬇️
... and 6 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 1e0d1bf...001e082. Read the comment docs.

server/lease.go

rleungx · 2019-08-15T07:25:10Z

server/lease.go

+	if cost := time.Since(start); cost > slowRequestTime {
+		log.Warn("lease grants too slow", zap.Duration("cost", cost))
+	}
+	if err != nil {


How about returning this error before line 51?

rleungx · 2019-08-15T08:25:46Z

server/lease.go

+func (l *LeaderLease) KeepAlive(ctx context.Context) {
+	ctx, cancel := context.WithCancel(ctx)
+	defer cancel()
+	timeCh := l.keepAliveWorker(ctx, l.leaseTimeout/3)


Why use 3 here?

Arbitrary value borrowed from etcd's keep alive code.

nolouch · 2019-08-15T09:17:54Z

server/lease.go

+
+// Close releases the lease.
+func (l *LeaderLease) Close() error {
+	return l.lease.Close()


May we need Revoke the lease before close? actually Close do not try to release the lease.

shafreeck

LGTM

Luffbee

LGTM

server/leader.go

Signed-off-by: disksing <i@disksing.com>

…ause

Signed-off-by: disksing <i@disksing.com>

disksing · 2019-08-19T11:24:47Z

/test

disksing · 2019-08-19T11:25:48Z

Please take another look. @rleungx @nolouch

nolouch

LGTM

disksing · 2019-08-19T12:21:32Z

/build

disksing · 2019-08-19T12:38:09Z

Created an issue for the test failure: #1693

* *: unify get store function everywhere (#1671) Signed-off-by: Ryan Leung <rleungx@gmail.com> * server: use leader lease to determine tso service validity (#1676) Signed-off-by: disksing <i@disksing.com> * test: fix tests (#1696) * test: fix region syncer test Signed-off-by: disksing <i@disksing.com> * add config-check flag for pd-server (#1695) Signed-off-by: cwen0 <cwenyin0@gmail.com> * operator: rewrite move region related functions (#1667) * *: support setting endKey for ScanRange (#1700) Signed-off-by: disksing <i@disksing.com> * *: reduce some unnecessary parameters (#1698) Signed-off-by: Ryan Leung <rleungx@gmail.com> * schedule: Do not send an operator of a region wth a stale epoch (#1659) * schedule: Do not send an operator of a region wth a stale epoch Signed-off-by: Shafreeck Sea <shafreeck@gmail.com> * schedule: check the version changed by the operator self Signed-off-by: Shafreeck Sea <shafreeck@gmail.com> * schedule: fix unit test Signed-off-by: Shafreeck Sea <shafreeck@gmail.com> * schedule: fix to avoid dispatching a stale opstep Signed-off-by: Shafreeck Sea <shafreeck@gmail.com> * dispatch: refactor "ConsumeConfVer() int" to "ExpectConfVerChange() bool" Signed-off-by: Shafreeck Sea <shafreeck@gmail.com> * dispatch: fix typo in comment Signed-off-by: Shafreeck Sea <shafreeck@gmail.com> * fix typo Co-Authored-By: Ryan Leung <rleungx@gmail.com> * dispatch: fix unittest Signed-off-by: Shafreeck Sea <shafreeck@gmail.com> * dispatch: refine format Signed-off-by: Shafreeck Sea <shafreeck@gmail.com> * server: fix the dead lock in scatter region (#1706) Signed-off-by: Ryan Leung <rleungx@gmail.com>

* *: unify get store function everywhere (#1671) Signed-off-by: Ryan Leung <rleungx@gmail.com> * remove unnecessary parentheses * server: use leader lease to determine tso service validity (#1676) Signed-off-by: disksing <i@disksing.com> * change internal stat values to float64 * add pending operator influence * add metrics of pending influence * fix metrics * fix panic * adjust pending influence of balanceHotWrite * change weight of pending influence * test: fix tests (#1696) * test: fix region syncer test Signed-off-by: disksing <i@disksing.com> * decrease region rolling window; store pending influence in scheduler * add config-check flag for pd-server (#1695) Signed-off-by: cwen0 <cwenyin0@gmail.com> * decrease possiblility transfer hot write leader * change pending influence weight * add unstarted op metrics * add logs for debug * add log for debug * add logs for debug * add logs for debug * add logs for debug * add logs for debug * add logs for debug * add logs for debug * Revert "add logs for debug" This reverts commit e74c7a9. * add metrics for hotspot operators * operator: rewrite move region related functions (#1667) * add metrics for pending operators * *: support setting endKey for ScanRange (#1700) Signed-off-by: disksing <i@disksing.com> * fix bug * fix bug * fix bug * fix metrics thread-safe bug * fix logic bug * *: reduce some unnecessary parameters (#1698) Signed-off-by: Ryan Leung <rleungx@gmail.com> * schedule: Do not send an operator of a region wth a stale epoch (#1659) * schedule: Do not send an operator of a region wth a stale epoch Signed-off-by: Shafreeck Sea <shafreeck@gmail.com> * schedule: check the version changed by the operator self Signed-off-by: Shafreeck Sea <shafreeck@gmail.com> * schedule: fix unit test Signed-off-by: Shafreeck Sea <shafreeck@gmail.com> * schedule: fix to avoid dispatching a stale opstep Signed-off-by: Shafreeck Sea <shafreeck@gmail.com> * dispatch: refactor "ConsumeConfVer() int" to "ExpectConfVerChange() bool" Signed-off-by: Shafreeck Sea <shafreeck@gmail.com> * dispatch: fix typo in comment Signed-off-by: Shafreeck Sea <shafreeck@gmail.com> * fix typo Co-Authored-By: Ryan Leung <rleungx@gmail.com> * dispatch: fix unittest Signed-off-by: Shafreeck Sea <shafreeck@gmail.com> * dispatch: refine format Signed-off-by: Shafreeck Sea <shafreeck@gmail.com> * server: fix the dead lock in scatter region (#1706) Signed-off-by: Ryan Leung <rleungx@gmail.com> * add drop time for operator * use IsDropped to recognize canceled ops * try to fix trans leader burst * try to fix trans leader burst * add zombie influence * change select src dst strategy; improve op_controller * change select src strategy * fix bug * tools: fix set namespace in pd-ctl (#1701) Signed-off-by: Ryan Leung <rleungx@gmail.com> * tools: fix parse url without http prefix (#1703) Signed-off-by: Ryan Leung <rleungx@gmail.com> * tests: support deadlock detection in make test (#1704) Signed-off-by: Ryan Leung <rleungx@gmail.com> * Makefile: fix failpoint enable (#1722) Signed-off-by: nolouch <nolouch@gmail.com> * checker: fix the issue that a region does not merge to the sibling with smaller size (#1723) Signed-off-by: disksing <i@disksing.com> * tools: balance region simulator (#1708) * scheduler: do not remove the operator when the step does not finish (#1715) Signed-off-by: Shafreeck Sea <shafreeck@gmail.com> * operator: fix the AddLearner config version judgment (#1732) Signed-off-by: nolouch <nolouch@gmail.com> * tools: fix TLS in pd control (#1729) Signed-off-by: Ryan Leung <rleungx@gmail.com> * syncer: support TLS for region syncer (#1728) Signed-off-by: Ryan Leung <rleungx@gmail.com> * schedule: fix a thread-safe bug and improve code (#1719)

nolouch · 2019-09-10T14:54:30Z

@disksing should we cherry pick to 3.0,3.1?

* *: unify get store function everywhere (#1671) Signed-off-by: Ryan Leung <rleungx@gmail.com> * server: use leader lease to determine tso service validity (#1676) Signed-off-by: disksing <i@disksing.com> * test: fix tests (#1696) * test: fix region syncer test Signed-off-by: disksing <i@disksing.com> * add config-check flag for pd-server (#1695) Signed-off-by: cwen0 <cwenyin0@gmail.com> * operator: rewrite move region related functions (#1667) * *: support setting endKey for ScanRange (#1700) Signed-off-by: disksing <i@disksing.com> * *: reduce some unnecessary parameters (#1698) Signed-off-by: Ryan Leung <rleungx@gmail.com> * schedule: Do not send an operator of a region wth a stale epoch (#1659) * schedule: Do not send an operator of a region wth a stale epoch Signed-off-by: Shafreeck Sea <shafreeck@gmail.com> * schedule: check the version changed by the operator self Signed-off-by: Shafreeck Sea <shafreeck@gmail.com> * schedule: fix unit test Signed-off-by: Shafreeck Sea <shafreeck@gmail.com> * schedule: fix to avoid dispatching a stale opstep Signed-off-by: Shafreeck Sea <shafreeck@gmail.com> * dispatch: refactor "ConsumeConfVer() int" to "ExpectConfVerChange() bool" Signed-off-by: Shafreeck Sea <shafreeck@gmail.com> * dispatch: fix typo in comment Signed-off-by: Shafreeck Sea <shafreeck@gmail.com> * fix typo Co-Authored-By: Ryan Leung <rleungx@gmail.com> * dispatch: fix unittest Signed-off-by: Shafreeck Sea <shafreeck@gmail.com> * dispatch: refine format Signed-off-by: Shafreeck Sea <shafreeck@gmail.com> * server: fix the dead lock in scatter region (#1706) Signed-off-by: Ryan Leung <rleungx@gmail.com> * tools: fix set namespace in pd-ctl (#1701) Signed-off-by: Ryan Leung <rleungx@gmail.com> * tools: fix parse url without http prefix (#1703) Signed-off-by: Ryan Leung <rleungx@gmail.com> * tests: support deadlock detection in make test (#1704) Signed-off-by: Ryan Leung <rleungx@gmail.com> * Makefile: fix failpoint enable (#1722) Signed-off-by: nolouch <nolouch@gmail.com> * checker: fix the issue that a region does not merge to the sibling with smaller size (#1723) Signed-off-by: disksing <i@disksing.com> * tools: balance region simulator (#1708) * scheduler: do not remove the operator when the step does not finish (#1715) Signed-off-by: Shafreeck Sea <shafreeck@gmail.com> * operator: fix the AddLearner config version judgment (#1732) Signed-off-by: nolouch <nolouch@gmail.com> * tools: fix TLS in pd control (#1729) Signed-off-by: Ryan Leung <rleungx@gmail.com> * syncer: support TLS for region syncer (#1728) Signed-off-by: Ryan Leung <rleungx@gmail.com> * schedule: fix a thread-safe bug and improve code (#1719) * statistics: fix region flow calculation (#1688) Signed-off-by: jiyingtk <jiyingtk@mail.ustc.edu.cn> * makefile: improve deadlock-enable/disable (#1736) * api: fix missing keys statistic in region information (#1741) Signed-off-by: nolouch <nolouch@gmail.com> * *: update go version to 1.13 (#1742) Signed-off-by: disksing <i@disksing.com> * coordinator: add the operator cost time in log field (#1748) Signed-off-by: nolouch <nolouch@gmail.com>

disksing · 2019-09-23T04:03:27Z

@nolouch I think no.

* server: use leader lease to determine tso service validity#1676 Signed-off-by: nolouch <nolouch@gmail.com>

disksing added 2 commits August 9, 2019 16:59

server: extract lease

9f86700

Signed-off-by: disksing <i@disksing.com>

server: TSO uses leader lease to determine validity

6e807a4

Signed-off-by: disksing <i@disksing.com>

disksing requested review from nolouch and rleungx August 9, 2019 11:33

Merge branch 'master' into tso-pause

001e082

disksing added status/can-merge Indicates a PR has been approved by a committer. and removed status/can-merge Indicates a PR has been approved by a committer. labels Aug 12, 2019

disksing requested a review from Luffbee August 14, 2019 08:18

nolouch added the priority/P1 The issue has P1 priority. label Aug 15, 2019

shafreeck reviewed Aug 15, 2019

View reviewed changes

server/lease.go Outdated Show resolved Hide resolved

rleungx reviewed Aug 15, 2019

View reviewed changes

nolouch reviewed Aug 15, 2019

View reviewed changes

shafreeck approved these changes Aug 15, 2019

View reviewed changes

Luffbee approved these changes Aug 15, 2019

View reviewed changes

server/leader.go Outdated Show resolved Hide resolved

nolouch added the status/LGT2 Indicates that a PR has LGTM 2. label Aug 16, 2019

disksing added 5 commits August 19, 2019 15:25

lease: grant fail fast

93febd1

Signed-off-by: disksing <i@disksing.com>

revoke before close

8ff9e1a

Signed-off-by: disksing <i@disksing.com>

Merge branch 'tso-pause' of https://github.com/disksing/pd into tso-p…

8198903

…ause

Merge branch 'master' into tso-pause

715fb22

fix conflicts

2500c6b

Signed-off-by: disksing <i@disksing.com>

Merge branch 'master' into tso-pause

a92050d

nolouch approved these changes Aug 19, 2019

View reviewed changes

rleungx approved these changes Aug 19, 2019

View reviewed changes

disksing merged commit 16d7464 into tikv:master Aug 19, 2019

disksing deleted the tso-pause branch August 19, 2019 12:42

disksing mentioned this pull request Aug 20, 2019

server: check if server is started when handling tso #1697

Closed

nolouch added the needs-cherry-pick-release-3.0 The PR needs to cherry pick to release-3.0 branch. label Sep 10, 2019

disksing removed the needs-cherry-pick-release-3.0 The PR needs to cherry pick to release-3.0 branch. label Sep 25, 2019

nolouch mentioned this pull request Feb 11, 2020

server: use leader lease to determine tso service validity #1676 #2117

Merged

nolouch added a commit that referenced this pull request Feb 14, 2020

server: use leader lease to determine tso service validity #1676 (#2117)

1b67bbb

* server: use leader lease to determine tso service validity#1676 Signed-off-by: nolouch <nolouch@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

server: use leader lease to determine tso service validity #1676

server: use leader lease to determine tso service validity #1676

disksing commented Aug 9, 2019 •

edited

codecov-io commented Aug 9, 2019

rleungx Aug 15, 2019

disksing Aug 19, 2019

rleungx Aug 15, 2019

disksing Aug 19, 2019

nolouch Aug 15, 2019 •

edited

shafreeck left a comment

Luffbee left a comment

disksing commented Aug 19, 2019

disksing commented Aug 19, 2019

nolouch left a comment

disksing commented Aug 19, 2019

disksing commented Aug 19, 2019

nolouch commented Sep 10, 2019

disksing commented Sep 23, 2019

server: use leader lease to determine tso service validity #1676

server: use leader lease to determine tso service validity #1676

Conversation

disksing commented Aug 9, 2019 • edited

What problem does this PR solve?

What is changed and how it works?

Check List

codecov-io commented Aug 9, 2019

Codecov Report

rleungx Aug 15, 2019

Choose a reason for hiding this comment

disksing Aug 19, 2019

Choose a reason for hiding this comment

rleungx Aug 15, 2019

Choose a reason for hiding this comment

disksing Aug 19, 2019

Choose a reason for hiding this comment

nolouch Aug 15, 2019 • edited

Choose a reason for hiding this comment

shafreeck left a comment

Choose a reason for hiding this comment

Luffbee left a comment

Choose a reason for hiding this comment

disksing commented Aug 19, 2019

disksing commented Aug 19, 2019

nolouch left a comment

Choose a reason for hiding this comment

disksing commented Aug 19, 2019

disksing commented Aug 19, 2019

nolouch commented Sep 10, 2019

disksing commented Sep 23, 2019

disksing commented Aug 9, 2019 •

edited

nolouch Aug 15, 2019 •

edited