*: use txn for saving timestamp #6199

rleungx · 2023-03-21T06:50:28Z

What problem does this PR solve?

Issue Number: Ref #5895.

What is changed and how does it work?

This PR supports using Txn to save the timestamp to prevent TSO from falling back when two places advance at the same time.

Check List

Tests

Unit test

Release note

None.

ti-chi-bot · 2023-03-21T06:50:29Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

JmPotato
lhy1024

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

JmPotato · 2023-03-21T07:46:34Z

/merge

ti-chi-bot · 2023-03-21T07:46:35Z

@JmPotato: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

ti-chi-bot · 2023-03-21T07:46:38Z

This pull request has been accepted and is ready to merge.

Commit hash: 087c36d

Signed-off-by: Ryan Leung <rleungx@gmail.com>

lhy1024 · 2023-03-21T09:42:16Z

ci failed

Signed-off-by: Ryan Leung <rleungx@gmail.com>

rleungx · 2023-03-21T10:15:00Z

/hold

binshi-bing

left a few comments.

binshi-bing · 2023-03-21T17:06:03Z

pkg/storage/endpoint/tso.go

+func (se *StorageEndpoint) SaveTimestamp(prefix string, key string, ts time.Time) error {
+	return se.RunInTxn(context.Background(), func(txn kv.Txn) error {
+		prefixEnd := clientv3.GetPrefixRangeEnd(prefix)
+		keys, values, err := txn.LoadRange(prefix, prefixEnd, 0)


If my understanding is correct, for global location, we use '/pd/[cluster-id]' to do prefix scan, but there are a lot of things, under this path, irrelevant to tso. In order to "“LoadTimestamp will get all time windows of Local/Global TSOs from etcd and return the biggest one.”, shall we use the /pd//timestamp/ for local TSOs?

binshi-bing · 2023-03-21T17:08:13Z

pkg/storage/endpoint/tso.go

+		previousTS := typeutil.ZeroTime
+		for i, key := range keys {
+			key := strings.TrimSpace(key)
+			if !strings.HasSuffix(key, timestampKey) {


Isn't this condition "!strings.HasSuffix(key, timestampKey)" too loose, considering we're using "/pd/[cluster-id]" to do prefix range scan? Could there be chance that 'timestamp' suffix are added to the path for other use cases?

binshi-bing · 2023-03-21T17:11:22Z

pkg/storage/endpoint/tso.go

+			return nil
+		}
+		data := typeutil.Uint64ToBytes(uint64(ts.UnixNano()))
+		return txn.Save(key, string(data))


Because LoadTimestamp() use the logic to do prefix scan then return the largest timestamp among all timestamps, it seems that we don't need the whole change in this pr -- tso service doesn't need to use the same path as pd's but the same prefix for timestamp range scan, then anyway LoadTimestamp() will return the largest one among the last writes of pd and tso service. Thus we can avoid using the expensive transaction here.

With this transaction, we scan all the dir and sub-dir of /pd/【cluster-id】 in a transaction, which is too expensive.

Signed-off-by: Ryan Leung <rleungx@gmail.com>

binshi-bing

LGTM

binshi-bing · 2023-03-21T18:07:11Z

pkg/storage/endpoint/tso.go

+			return nil
+		}
+		data := typeutil.Uint64ToBytes(uint64(ts.UnixNano()))
+		return txn.Save(key, string(data))


With this transaction, we scan all the dir and sub-dir of /pd/【cluster-id】 in a transaction, which is too expensive.

binshi-bing · 2023-03-21T18:26:45Z

pkg/storage/endpoint/tso.go

+		}
+		data := typeutil.Uint64ToBytes(uint64(ts.UnixNano()))
+		return txn.Save(key, string(data))
+	})


Without this pr，even LoadTimestamp is invoked only once after it becomes the leader/primary，when we switch service mode, we need to ensure the it campaigns to be the leader/primary then invoke LoadTimestamp() to load the largest one among the last writes of pd and tso service which use the same prefix but different paths.

already change to Load which will only get one key.

binshi-bing · 2023-03-22T04:16:26Z

pkg/storage/endpoint/tso.go

+		}
+
+		previousTS := typeutil.ZeroTime
+		if value != "" {


Suggested change

if value != "" {

if len(value) > 0 {

I think there is not much difference.

binshi-bing · 2023-03-22T04:23:24Z

pkg/storage/endpoint/tso.go

+			}
+		}
+		if previousTS != typeutil.ZeroTime && typeutil.SubRealTimeByWallClock(ts, previousTS) <= 0 {
+			log.Warn("save timestamp failed, the timestamp is not bigger than the previous one", zap.Time("previous", previousTS), zap.Time("current", ts))


Do we need this log line? It seems to be a normal case.

codecov · 2023-03-22T04:33:25Z

Codecov Report

Patch coverage: 78.57% and project coverage change: +0.04 🎉

Comparison is base (738e15f) 74.77% compared to head (b4d822a) 74.81%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6199      +/-   ##
==========================================
+ Coverage   74.77%   74.81%   +0.04%     
==========================================
  Files         395      395              
  Lines       38704    38716      +12     
==========================================
+ Hits        28939    28966      +27     
+ Misses       7232     7225       -7     
+ Partials     2533     2525       -8

Flag	Coverage Δ
unittests	`74.81% <78.57%> (+0.04%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pkg/storage/endpoint/tso.go	`83.87% <78.57%> (+4.92%)`	⬆️

... and 22 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Signed-off-by: Ryan Leung <rleungx@gmail.com>

rleungx · 2023-03-22T05:25:18Z

/hold cancel

rleungx · 2023-03-22T06:02:04Z

/merge

ti-chi-bot · 2023-03-22T06:02:06Z

@rleungx: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

ti-chi-bot · 2023-03-22T06:02:07Z

This pull request has been accepted and is ready to merge.

Commit hash: d491ede

ti-chi-bot · 2023-03-22T06:02:21Z

@rleungx: Your PR was out of date, I have automatically updated it for you.

If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

lhy1024 · 2023-03-22T06:35:41Z

ci failed

rleungx · 2023-03-22T06:48:31Z

/hold

rleungx · 2023-03-22T06:56:48Z

/hold cancel

ti-chi-bot added the release-note-none Denotes a PR that doesn't merit a release note. label Mar 21, 2023

ti-chi-bot requested review from lhy1024 and nolouch March 21, 2023 06:50

lhy1024 approved these changes Mar 21, 2023

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Mar 21, 2023

rleungx removed the request for review from nolouch March 21, 2023 07:08

rleungx force-pushed the tso-txn branch 2 times, most recently from a7a59b2 to 087c36d Compare March 21, 2023 07:14

rleungx requested a review from JmPotato March 21, 2023 07:20

JmPotato approved these changes Mar 21, 2023

View reviewed changes

ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Mar 21, 2023

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Mar 21, 2023

use txn for saving timestamp

46afca4

Signed-off-by: Ryan Leung <rleungx@gmail.com>

tiny fix

d209006

Signed-off-by: Ryan Leung <rleungx@gmail.com>

rleungx force-pushed the tso-txn branch from aaeb551 to d209006 Compare March 21, 2023 10:13

ti-chi-bot removed the status/can-merge Indicates a PR has been approved by a committer. label Mar 21, 2023

ti-chi-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 21, 2023

binshi-bing reviewed Mar 21, 2023

View reviewed changes

rleungx added 2 commits March 22, 2023 12:07

use load instead of loadrange

29b9283

Signed-off-by: Ryan Leung <rleungx@gmail.com>

tiny fix

1bc2f64

Signed-off-by: Ryan Leung <rleungx@gmail.com>

binshi-bing approved these changes Mar 22, 2023

View reviewed changes

typo

d491ede

Signed-off-by: Ryan Leung <rleungx@gmail.com>

ti-chi-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 22, 2023

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Mar 22, 2023

Merge branch 'master' into tso-txn

b4d822a

ti-chi-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 22, 2023

ti-chi-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 22, 2023

ti-chi-bot merged commit 27b9474 into tikv:master Mar 22, 2023

rleungx deleted the tso-txn branch March 22, 2023 07:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: use txn for saving timestamp #6199

*: use txn for saving timestamp #6199

rleungx commented Mar 21, 2023

ti-chi-bot commented Mar 21, 2023 •

edited

Loading

JmPotato commented Mar 21, 2023

ti-chi-bot commented Mar 21, 2023

ti-chi-bot commented Mar 21, 2023

lhy1024 commented Mar 21, 2023

rleungx commented Mar 21, 2023

binshi-bing left a comment

binshi-bing Mar 21, 2023

binshi-bing Mar 21, 2023

binshi-bing Mar 21, 2023 •

edited

Loading

binshi-bing Mar 21, 2023

binshi-bing left a comment

binshi-bing Mar 21, 2023

binshi-bing Mar 21, 2023

rleungx Mar 22, 2023

binshi-bing Mar 22, 2023

rleungx Mar 22, 2023

binshi-bing Mar 22, 2023

codecov bot commented Mar 22, 2023 •

edited

Loading

rleungx commented Mar 22, 2023

rleungx commented Mar 22, 2023

ti-chi-bot commented Mar 22, 2023

ti-chi-bot commented Mar 22, 2023

ti-chi-bot commented Mar 22, 2023

lhy1024 commented Mar 22, 2023

rleungx commented Mar 22, 2023

rleungx commented Mar 22, 2023

*: use txn for saving timestamp #6199

*: use txn for saving timestamp #6199

Conversation

rleungx commented Mar 21, 2023

What problem does this PR solve?

What is changed and how does it work?

Check List

Release note

ti-chi-bot commented Mar 21, 2023 • edited Loading

JmPotato commented Mar 21, 2023

ti-chi-bot commented Mar 21, 2023

ti-chi-bot commented Mar 21, 2023

lhy1024 commented Mar 21, 2023

rleungx commented Mar 21, 2023

binshi-bing left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

binshi-bing Mar 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

binshi-bing left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Mar 22, 2023 • edited Loading

Codecov Report

rleungx commented Mar 22, 2023

rleungx commented Mar 22, 2023

ti-chi-bot commented Mar 22, 2023

ti-chi-bot commented Mar 22, 2023

ti-chi-bot commented Mar 22, 2023

lhy1024 commented Mar 22, 2023

rleungx commented Mar 22, 2023

rleungx commented Mar 22, 2023

ti-chi-bot commented Mar 21, 2023 •

edited

Loading

binshi-bing Mar 21, 2023 •

edited

Loading

codecov bot commented Mar 22, 2023 •

edited

Loading