server: alloc timestamp after recover #1173

rleungx · 2018-08-01T11:18:06Z

What have you changed? (required)

This PR allows allocating timestamp after recovering from the incorrect system time.

What are the type of the changes (required)?

Improvement (non-breaking change which is an improvement to an existing feature)

How has this PR been tested (required)?

Manual tests

Refer to a related PR or issue link (optional)

#499, #940, pingcap/tidb#5709.

Benchmark result if necessary (optional)

Add a few positive/negative examples (optional)

disksing · 2018-08-01T11:31:43Z

server/tso.go

+	if subTimeByWallClock(last, now)+updateTimestampGuard > 0 {
+		log.Errorf("system time may be incorrect, last: %v, now: %v", last, now)
+		s.ts.Store(&atomicObject{physical: last})
+		return nil


I think you want to update now and save it to etcd.

disksing · 2018-08-02T06:39:14Z

server/tso.go

-			continue
+			// If the time window is enough, it will increase the physical time,
+			// otherwise, it will wait for updating the time window.
+			if subTimeByWallClock(s.lastSavedTime, current.physical) > updateTimestampGuard {


I think lastSavedTime is accessed concurrently without any mutex.

nolouch · 2018-08-02T07:32:49Z

server/tso.go

-		log.Debugf("save timestamp ok: prev %v last %v save %v", prev, last, save)
+		// If the time window is no more than updateTimestampGuard,
+		// it will adjust the time window.
+		if subTimeByWallClock(saved, now) <= updateTimestampGuard {


better also validate the TsoSaveInterval large than updateTimestampGuard in config.

They are not in the same order of magnitude, and thus it doesn't make much sense. What do you think?

Oh, It's ok. I before thought the updateTimestampGuard was 1s.

nolouch · 2018-08-02T07:44:25Z

server/tso.go

+		}
+		// Since the physical timestamp is greater than the current system time,
+		// the physical timestamp will be used to alloc timestamp temporarily.
+		current.physical = prev


The logical part will clean to 0, but you do not increase the physical part.

nolouch · 2018-08-02T08:09:32Z

PTAL @liukun4515

disksing · 2018-08-02T08:35:39Z

server/tso.go

+					physical: current.physical.Add(time.Millisecond),
+					logical:  0,
+				}
+				s.ts.Store(last)


miss a continue?

iamxy · 2018-08-02T10:15:55Z

/rebuild

disksing · 2018-08-02T12:02:34Z

I think we can refactor updateTimestamp() like this:

func updateTimestamp() {
    var next int64                             // step1. determine next physical timestamp
    if now > prev.physical + 1ms {            // a. if system is greater, sync with system time
        next = now
    }
    else if prev.logical > maxLogical/2 {          // b. if logical is about to use up, increase physical by 1ms
        next = prev.physical+1ms
    } else {                                      // otherwise the physical does not need update
        return
    }

    if saved - next <= 1ms {                  // step2. determine if we need to save ts to etcd
        saveTimestamp(next + 3s)          // when it is unsafe to increase physical to `next`, we save etcd so the saved will increase by 3s
    }

    s.ts.Store({next, 0})        // step3. update physical
    return nil
}

disksing · 2018-08-03T01:18:13Z

I think we need more detailed comments.

disksing · 2018-08-03T02:01:49Z

server/tso.go

+		next = now
+	} else if atomic.LoadInt64(&prev.logical) > maxLogical/2 {
+		next = prev.physical.Add(time.Millisecond)
+	} else {


tsoCounter.WithLabelValues("skip_save").Inc() before return nil

disksing · 2018-08-03T02:02:41Z

server/tso.go

+	if jetLag > updateTimestampGuard {
+		next = now
+	} else if atomic.LoadInt64(&prev.logical) > maxLogical/2 {
+		next = prev.physical.Add(time.Millisecond)


Add warning log here.

disksing · 2018-08-03T02:05:58Z

server/tso.go

+	// the timestamp allocation will start from the saved etcd timestamp temporarily.
+	if subTimeByWallClock(next, last) < updateTimestampGuard {
+		log.Errorf("system time may be incorrect: last: %v next %v", last, next)
+		next = next.Add(updateTimestampGuard)


next = last.Add(updateTimestampGuard)?

disksing · 2018-08-03T02:09:08Z

Please review this PR carefully. /cc @nolouch @siddontang @liukun4515

rleungx · 2018-08-03T03:00:39Z

@disksing PTAL.

disksing · 2018-08-03T11:13:46Z

server/tso.go

+//
+// Here is some constraints that this function must satisfy:
+// 1. The physical time is monotonically increasing.
+// 2. The saved time is monotonically increasing.


And the physical should always less than saved timestamp.

nolouch

LGTM

siddontang · 2018-08-06T01:26:18Z

please construct a test for the time fallback.

siddontang

LGTM

siddontang · 2018-08-07T14:26:33Z

why add DNM label again? @rleungx

rleungx · 2018-08-08T02:01:11Z

There is a concurrency problem in the test. @siddontang

rleungx · 2018-08-09T05:41:03Z

The test will be added in another PR. I will create it once this PR is merged.

rleungx changed the title ~~alloc timestamp after recover~~ simulator: alloc timestamp after recover Aug 1, 2018

rleungx added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 1, 2018

rleungx force-pushed the tso-no-wait branch from f18bacf to 2b92e8c Compare August 1, 2018 11:26

rleungx changed the title ~~simulator: alloc timestamp after recover~~ server: alloc timestamp after recover Aug 1, 2018

disksing reviewed Aug 1, 2018

View reviewed changes

rleungx force-pushed the tso-no-wait branch from 2b92e8c to 7d54310 Compare August 2, 2018 06:19

rleungx requested a review from nolouch August 2, 2018 06:31

disksing reviewed Aug 2, 2018

View reviewed changes

nolouch reviewed Aug 2, 2018

View reviewed changes

rleungx removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 2, 2018

disksing reviewed Aug 2, 2018

View reviewed changes

server/tso.go Outdated

physical: current.physical.Add(time.Millisecond),

logical: 0,

}

s.ts.Store(last)

Copy link

Contributor

disksing Aug 2, 2018

Choose a reason for hiding this comment

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

miss a continue?

rleungx added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 2, 2018

rleungx force-pushed the tso-no-wait branch 4 times, most recently from 154689e to 2daa77f Compare August 2, 2018 13:45

rleungx removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 2, 2018

disksing reviewed Aug 3, 2018

View reviewed changes

rleungx force-pushed the tso-no-wait branch 5 times, most recently from 14ff8f2 to 4e9fd5b Compare August 3, 2018 02:52

disksing reviewed Aug 3, 2018

View reviewed changes

nolouch reviewed Aug 3, 2018

View reviewed changes

rleungx force-pushed the tso-no-wait branch 3 times, most recently from 3c15dc1 to 8ae950c Compare August 6, 2018 06:18

disksing approved these changes Aug 6, 2018

View reviewed changes

siddontang approved these changes Aug 7, 2018

View reviewed changes

nolouch approved these changes Aug 7, 2018

View reviewed changes

rleungx force-pushed the tso-no-wait branch from 8ae950c to d56c2ec Compare August 7, 2018 10:27

rleungx added the DNM label Aug 7, 2018

rleungx added 9 commits August 9, 2018 11:20

alloc timestamp after recover

42efbcd

move saveTimestamp into updateTimestamp

550326a

change lastSavedTime to be atomic.Value

0fa9b46

avoid allocating duplicated ts

602e6d4

avoid the concurrency problem

005d6dc

refactor updateTimestamp according to the comment

4210546

address comments

1491c69

add the metrics and the test

7f6e745

remove test

8195bf8

rleungx force-pushed the tso-no-wait branch from d56c2ec to 8195bf8 Compare August 9, 2018 05:30

rleungx removed the DNM label Aug 9, 2018

disksing merged commit aea8e1c into tikv:master Aug 9, 2018

rleungx mentioned this pull request Aug 9, 2018

server: use gofail to test time fallback #1186

Merged

nolouch pushed a commit to nolouch/pd that referenced this pull request Aug 15, 2018

server: alloc timestamp after recover (tikv#1173)

28c8da4

rleungx mentioned this pull request Aug 24, 2018

pd: alert time fallback pingcap/tidb-ansible#511

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

server: alloc timestamp after recover #1173

server: alloc timestamp after recover #1173

rleungx commented Aug 1, 2018 •

edited

Loading

disksing Aug 1, 2018 •

edited

Loading

disksing Aug 2, 2018

rleungx Aug 2, 2018

nolouch Aug 2, 2018

rleungx Aug 2, 2018

nolouch Aug 2, 2018 •

edited

Loading

nolouch Aug 2, 2018

nolouch commented Aug 2, 2018

disksing Aug 2, 2018

iamxy commented Aug 2, 2018

disksing commented Aug 2, 2018 •

edited

Loading

disksing commented Aug 3, 2018

disksing Aug 3, 2018

disksing Aug 3, 2018

disksing Aug 3, 2018

disksing commented Aug 3, 2018

rleungx commented Aug 3, 2018

disksing Aug 3, 2018

nolouch left a comment

siddontang commented Aug 6, 2018

siddontang left a comment

siddontang commented Aug 7, 2018

rleungx commented Aug 8, 2018

rleungx commented Aug 9, 2018

server: alloc timestamp after recover #1173

server: alloc timestamp after recover #1173

Conversation

rleungx commented Aug 1, 2018 • edited Loading

What have you changed? (required)

What are the type of the changes (required)?

How has this PR been tested (required)?

Refer to a related PR or issue link (optional)

Benchmark result if necessary (optional)

Add a few positive/negative examples (optional)

disksing Aug 1, 2018 • edited Loading

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

nolouch Aug 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nolouch commented Aug 2, 2018

Choose a reason for hiding this comment

iamxy commented Aug 2, 2018

disksing commented Aug 2, 2018 • edited Loading

disksing commented Aug 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

disksing commented Aug 3, 2018

rleungx commented Aug 3, 2018

Choose a reason for hiding this comment

nolouch left a comment

Choose a reason for hiding this comment

siddontang commented Aug 6, 2018

siddontang left a comment

Choose a reason for hiding this comment

siddontang commented Aug 7, 2018

rleungx commented Aug 8, 2018

rleungx commented Aug 9, 2018

rleungx commented Aug 1, 2018 •

edited

Loading

disksing Aug 1, 2018 •

edited

Loading

nolouch Aug 2, 2018 •

edited

Loading

disksing commented Aug 2, 2018 •

edited

Loading