From dc52986449dcdc200b46c6990b4ed00204a9ae1d Mon Sep 17 00:00:00 2001 From: MyonKeminta Date: Wed, 19 Dec 2018 13:04:15 +0800 Subject: [PATCH 1/4] Add compatible parse time Signed-off-by: MyonKeminta --- store/tikv/gcworker/gc_worker.go | 29 ++++++++++++++++++-- store/tikv/gcworker/gc_worker_test.go | 38 +++++++++++++++++++++++++++ 2 files changed, 65 insertions(+), 2 deletions(-) diff --git a/store/tikv/gcworker/gc_worker.go b/store/tikv/gcworker/gc_worker.go index bf096f3ca73cc..19481ca5b352d 100644 --- a/store/tikv/gcworker/gc_worker.go +++ b/store/tikv/gcworker/gc_worker.go @@ -95,7 +95,7 @@ func (w *GCWorker) Close() { } const ( - gcTimeFormat = "20060102-15:04:05 -0700 MST" + gcTimeFormat = "20060102-15:04:05 -0700" gcWorkerTickInterval = time.Minute gcJobLogTickInterval = time.Minute * 10 gcWorkerLease = time.Minute * 2 @@ -1078,8 +1078,33 @@ func NewMockGCWorker(store tikv.Storage) (*MockGCWorker, error) { return &MockGCWorker{worker: worker}, nil } -// DeleteRanges call deleteRanges internally, just for test. +// DeleteRanges calls deleteRanges internally, just for test. func (w *MockGCWorker) DeleteRanges(ctx context.Context, safePoint uint64) error { log.Errorf("deleteRanges is called") return w.worker.deleteRanges(ctx, safePoint) } + +// ParseTime parses a time string to time.Time. +// We used to use "20060102-15:04:05 -0700 MST" as the time format to save safe point and last gc run time. However, we +// found that in some environments, it can't correctly print a time zone name like "CST", but producing something like +// "+08". Since the information of time zone is already represented in the field "-0700", it's ok to remove "MST". Then, +// we need to accept both new format and old format for compatibility. +func CompatibleParseTime(value string) (time.Time, error) { + t, err := time.Parse(gcTimeFormat, value) + + if err != nil { + // Cut from the last space + i := len(value) + for i > 0 { + i-- + if value[i] == ' ' { + break + } + } + + // It doesn't matter if `value` is empty or contains no space character. `time.Parse` will return the error. + t, err = time.Parse(gcTimeFormat, value[0:i]) + } + + return t, err +} diff --git a/store/tikv/gcworker/gc_worker_test.go b/store/tikv/gcworker/gc_worker_test.go index d6ddba6fce096..c8ca44e53e8d1 100644 --- a/store/tikv/gcworker/gc_worker_test.go +++ b/store/tikv/gcworker/gc_worker_test.go @@ -213,3 +213,41 @@ func (s *testGCWorkerSuite) TestDoGC(c *C) { err = s.gcWorker.doGC(ctx, 20) c.Assert(err, IsNil) } + +func (s *testGCWorkerSuite) TestCompatibleParseTime(c *C) { + // The old format: "20060102-15:04:05 -0700 MST" + + values := []string{ + "20181218-19:53:37 +0800 CST", + "20181218-19:53:37 +0800 +08", + "20181218-19:53:37 +0800", + "20181218-11:53:37 +0000", + } + + invalidValues := []string{ + "", + " ", + "foo", + "20181218-11:53:37", + } + + expectedTime := time.Date(2018, 12, 18, 11, 53, 37, 0, time.UTC) + expectedTimeFormatted := "20181218-19:53:37 +0800" + + beijing, err := time.LoadLocation("Asia/Shanghai") + c.Assert(err, IsNil) + + for _, value := range values { + t, err := CompatibleParseTime(value) + c.Assert(err, IsNil) + c.Assert(t.Equal(expectedTime), Equals, true) + + formatted := t.In(beijing).Format(gcTimeFormat) + c.Assert(formatted, Equals, expectedTimeFormatted) + } + + for _, value := range invalidValues { + _, err := CompatibleParseTime(value) + c.Assert(err, NotNil) + } +} From bebfd2224731ac1bc64b3af76da26a06b14f2af0 Mon Sep 17 00:00:00 2001 From: MyonKeminta Date: Wed, 19 Dec 2018 14:30:13 +0800 Subject: [PATCH 2/4] Use new formatting Signed-off-by: MyonKeminta --- executor/admin_test.go | 2 +- executor/executor_test.go | 2 +- executor/set.go | 6 ++-- store/tikv/gcworker/gc_worker.go | 27 +-------------- store/tikv/gcworker/gc_worker_test.go | 38 --------------------- util/misc.go | 25 ++++++++++++++ util/misc_test.go | 49 ++++++++++++++++++++++++++- 7 files changed, 79 insertions(+), 70 deletions(-) diff --git a/executor/admin_test.go b/executor/admin_test.go index 81d3a58e85c8c..3648ec93418dd 100644 --- a/executor/admin_test.go +++ b/executor/admin_test.go @@ -594,7 +594,7 @@ func (s *testSuite) TestAdminCheckWithSnapshot(c *C) { // For mocktikv, safe point is not initialized, we manually insert it for snapshot to use. safePointName := "tikv_gc_safe_point" - safePointValue := "20060102-15:04:05 -0700 MST" + safePointValue := "20060102-15:04:05 -0700" safePointComment := "All versions after safe point can be accessed. (DO NOT EDIT)" updateSafePoint := fmt.Sprintf(`INSERT INTO mysql.tidb VALUES ('%[1]s', '%[2]s', '%[3]s') ON DUPLICATE KEY diff --git a/executor/executor_test.go b/executor/executor_test.go index 52430505bb534..83fdcf03efdc5 100644 --- a/executor/executor_test.go +++ b/executor/executor_test.go @@ -1973,7 +1973,7 @@ func (s *testSuite) TestHistoryRead(c *C) { // For mocktikv, safe point is not initialized, we manually insert it for snapshot to use. safePointName := "tikv_gc_safe_point" - safePointValue := "20060102-15:04:05 -0700 MST" + safePointValue := "20060102-15:04:05 -0700" safePointComment := "All versions after safe point can be accessed. (DO NOT EDIT)" updateSafePoint := fmt.Sprintf(`INSERT INTO mysql.tidb VALUES ('%[1]s', '%[2]s', '%[3]s') ON DUPLICATE KEY diff --git a/executor/set.go b/executor/set.go index 84e329792b1ed..34182735947b5 100644 --- a/executor/set.go +++ b/executor/set.go @@ -17,7 +17,6 @@ import ( "context" "fmt" "strings" - "time" "github.com/pingcap/errors" "github.com/pingcap/parser/ast" @@ -28,6 +27,7 @@ import ( "github.com/pingcap/tidb/sessionctx" "github.com/pingcap/tidb/sessionctx/variable" "github.com/pingcap/tidb/types" + "github.com/pingcap/tidb/util" "github.com/pingcap/tidb/util/chunk" "github.com/pingcap/tidb/util/sqlexec" log "github.com/sirupsen/logrus" @@ -194,8 +194,8 @@ func validateSnapshot(ctx sessionctx.Context, snapshotTS uint64) error { return errors.New("can not get 'tikv_gc_safe_point'") } safePointString := rows[0].GetString(0) - const gcTimeFormat = "20060102-15:04:05 -0700 MST" - safePointTime, err := time.Parse(gcTimeFormat, safePointString) + const gcTimeFormat = "20060102-15:04:05 -0700" + safePointTime, err := util.ParseTimeFromPrefix(gcTimeFormat, safePointString) if err != nil { return errors.Trace(err) } diff --git a/store/tikv/gcworker/gc_worker.go b/store/tikv/gcworker/gc_worker.go index 19481ca5b352d..defbdbee00882 100644 --- a/store/tikv/gcworker/gc_worker.go +++ b/store/tikv/gcworker/gc_worker.go @@ -944,7 +944,7 @@ func (w *GCWorker) loadTime(key string) (*time.Time, error) { if str == "" { return nil, nil } - t, err := time.Parse(gcTimeFormat, str) + t, err := tidbutil.ParseTimeFromPrefix(gcTimeFormat, str) if err != nil { return nil, errors.Trace(err) } @@ -1083,28 +1083,3 @@ func (w *MockGCWorker) DeleteRanges(ctx context.Context, safePoint uint64) error log.Errorf("deleteRanges is called") return w.worker.deleteRanges(ctx, safePoint) } - -// ParseTime parses a time string to time.Time. -// We used to use "20060102-15:04:05 -0700 MST" as the time format to save safe point and last gc run time. However, we -// found that in some environments, it can't correctly print a time zone name like "CST", but producing something like -// "+08". Since the information of time zone is already represented in the field "-0700", it's ok to remove "MST". Then, -// we need to accept both new format and old format for compatibility. -func CompatibleParseTime(value string) (time.Time, error) { - t, err := time.Parse(gcTimeFormat, value) - - if err != nil { - // Cut from the last space - i := len(value) - for i > 0 { - i-- - if value[i] == ' ' { - break - } - } - - // It doesn't matter if `value` is empty or contains no space character. `time.Parse` will return the error. - t, err = time.Parse(gcTimeFormat, value[0:i]) - } - - return t, err -} diff --git a/store/tikv/gcworker/gc_worker_test.go b/store/tikv/gcworker/gc_worker_test.go index c8ca44e53e8d1..d6ddba6fce096 100644 --- a/store/tikv/gcworker/gc_worker_test.go +++ b/store/tikv/gcworker/gc_worker_test.go @@ -213,41 +213,3 @@ func (s *testGCWorkerSuite) TestDoGC(c *C) { err = s.gcWorker.doGC(ctx, 20) c.Assert(err, IsNil) } - -func (s *testGCWorkerSuite) TestCompatibleParseTime(c *C) { - // The old format: "20060102-15:04:05 -0700 MST" - - values := []string{ - "20181218-19:53:37 +0800 CST", - "20181218-19:53:37 +0800 +08", - "20181218-19:53:37 +0800", - "20181218-11:53:37 +0000", - } - - invalidValues := []string{ - "", - " ", - "foo", - "20181218-11:53:37", - } - - expectedTime := time.Date(2018, 12, 18, 11, 53, 37, 0, time.UTC) - expectedTimeFormatted := "20181218-19:53:37 +0800" - - beijing, err := time.LoadLocation("Asia/Shanghai") - c.Assert(err, IsNil) - - for _, value := range values { - t, err := CompatibleParseTime(value) - c.Assert(err, IsNil) - c.Assert(t.Equal(expectedTime), Equals, true) - - formatted := t.In(beijing).Format(gcTimeFormat) - c.Assert(formatted, Equals, expectedTimeFormatted) - } - - for _, value := range invalidValues { - _, err := CompatibleParseTime(value) - c.Assert(err, NotNil) - } -} diff --git a/util/misc.go b/util/misc.go index 3060f610c687d..f2d298ac6c753 100644 --- a/util/misc.go +++ b/util/misc.go @@ -72,3 +72,28 @@ func WithRecovery(exec func(), recoverFn func(r interface{})) { }() exec() } + +// ParseTimeFromPrefix tries to parse time from a prefix of `value`. The formatted time should be separated from +// other texts. For example, you can parse "20060102-15:04:05 -0700" from a string like "20060102-15:04:05 -0700 FOO" +// but you can't parse from a string like "20060102-15:04:05 -0700FOO". +func ParseTimeFromPrefix(format string, value string) (time.Time, error) { + t, err := time.Parse(format, value) + + i := len(value) + for err != nil && i > 0 { + // Cut from the last space + for i > 0 { + i-- + if value[i] == ' ' { + break + } + } + + t, err = time.Parse(format, value[0:i]) + } + + if err != nil { + err = errors.Errorf("string \"%v\" doesn't has a prefix that matches format \"%v\"", value, format) + } + return t, err +} diff --git a/util/misc_test.go b/util/misc_test.go index 5060d25278ada..ad7b90c09d9d1 100644 --- a/util/misc_test.go +++ b/util/misc_test.go @@ -14,6 +14,8 @@ package util import ( + "time" + . "github.com/pingcap/check" "github.com/pingcap/errors" "github.com/pingcap/tidb/util/testleak" @@ -30,7 +32,7 @@ func (s *testMiscSuite) SetUpSuite(c *C) { func (s *testMiscSuite) TearDownSuite(c *C) { } -func (s testMiscSuite) TestRunWithRetry(c *C) { +func (s *testMiscSuite) TestRunWithRetry(c *C) { defer testleak.AfterTest(c)() // Run succ. cnt := 0 @@ -68,3 +70,48 @@ func (s testMiscSuite) TestRunWithRetry(c *C) { c.Assert(err, NotNil) c.Assert(cnt, Equals, 1) } + +func (s *testMiscSuite) TestParseTimeFromPrefix(c *C) { + format := "20060102-15:04:05 -0700" + + values := []string{ + "20181218-19:53:37 +0800 CST", + "20181218-19:53:37 +0800 MST ", + "20181218-19:53:37 +0800 FOO", + "20181218-19:53:37 +0800 aaa bbb ccc 1 2 3 ", + "20181218-19:53:37 +0800 +08", + "20181218-19:53:37 +0800", + "20181218-19:53:37 +0800 ", + "20181218-11:53:37 +0000", + } + + invalidValues := []string{ + "", + " ", + "foo", + "20181218-11:53:37", + "20181218-19:53:37 +0800CST", + "20181218-19:53:37 +0800FOOOOOOO BAR", + "20181218-19:53:37 ", + } + + expectedTime := time.Date(2018, 12, 18, 11, 53, 37, 0, time.UTC) + expectedTimeFormatted := "20181218-19:53:37 +0800" + + beijing, err := time.LoadLocation("Asia/Shanghai") + c.Assert(err, IsNil) + + for _, value := range values { + t, err := ParseTimeFromPrefix(format, value) + c.Assert(err, IsNil) + c.Assert(t.Equal(expectedTime), Equals, true) + + formatted := t.In(beijing).Format(format) + c.Assert(formatted, Equals, expectedTimeFormatted) + } + + for _, value := range invalidValues { + _, err := ParseTimeFromPrefix(format, value) + c.Assert(err, NotNil) + } +} From 934515678088cd939c63b5bbd6bce6ec66b427eb Mon Sep 17 00:00:00 2001 From: MyonKeminta Date: Wed, 26 Dec 2018 11:50:31 +0800 Subject: [PATCH 3/4] address comments Signed-off-by: MyonKeminta --- executor/set.go | 3 +-- store/tikv/gcworker/gc_worker.go | 5 ++--- util/misc.go | 32 +++++++++++++++----------------- util/misc_test.go | 14 ++++++-------- 4 files changed, 24 insertions(+), 30 deletions(-) diff --git a/executor/set.go b/executor/set.go index 34182735947b5..9b67a749b735a 100644 --- a/executor/set.go +++ b/executor/set.go @@ -194,8 +194,7 @@ func validateSnapshot(ctx sessionctx.Context, snapshotTS uint64) error { return errors.New("can not get 'tikv_gc_safe_point'") } safePointString := rows[0].GetString(0) - const gcTimeFormat = "20060102-15:04:05 -0700" - safePointTime, err := util.ParseTimeFromPrefix(gcTimeFormat, safePointString) + safePointTime, err := util.CompatibleParseGCTime(safePointString) if err != nil { return errors.Trace(err) } diff --git a/store/tikv/gcworker/gc_worker.go b/store/tikv/gcworker/gc_worker.go index defbdbee00882..296613c52d283 100644 --- a/store/tikv/gcworker/gc_worker.go +++ b/store/tikv/gcworker/gc_worker.go @@ -95,7 +95,6 @@ func (w *GCWorker) Close() { } const ( - gcTimeFormat = "20060102-15:04:05 -0700" gcWorkerTickInterval = time.Minute gcJobLogTickInterval = time.Minute * 10 gcWorkerLease = time.Minute * 2 @@ -932,7 +931,7 @@ func (w *GCWorker) saveSafePoint(kv tikv.SafePointKV, key string, t uint64) erro } func (w *GCWorker) saveTime(key string, t time.Time) error { - err := w.saveValueToSysTable(key, t.Format(gcTimeFormat)) + err := w.saveValueToSysTable(key, t.Format(tidbutil.GcTimeFormat)) return errors.Trace(err) } @@ -944,7 +943,7 @@ func (w *GCWorker) loadTime(key string) (*time.Time, error) { if str == "" { return nil, nil } - t, err := tidbutil.ParseTimeFromPrefix(gcTimeFormat, str) + t, err := tidbutil.CompatibleParseGCTime(str) if err != nil { return nil, errors.Trace(err) } diff --git a/util/misc.go b/util/misc.go index f2d298ac6c753..8da6b35e02630 100644 --- a/util/misc.go +++ b/util/misc.go @@ -15,6 +15,7 @@ package util import ( "runtime" + "strings" "time" "github.com/pingcap/errors" @@ -26,6 +27,8 @@ const ( DefaultMaxRetries = 30 // RetryInterval indicates retry interval. RetryInterval uint64 = 500 + // GcTimeFormat is the format that gc_worker used to store times. + GcTimeFormat = "20060102-15:04:05 -0700" ) // RunWithRetry will run the f with backoff and retry. @@ -73,27 +76,22 @@ func WithRecovery(exec func(), recoverFn func(r interface{})) { exec() } -// ParseTimeFromPrefix tries to parse time from a prefix of `value`. The formatted time should be separated from -// other texts. For example, you can parse "20060102-15:04:05 -0700" from a string like "20060102-15:04:05 -0700 FOO" -// but you can't parse from a string like "20060102-15:04:05 -0700FOO". -func ParseTimeFromPrefix(format string, value string) (time.Time, error) { - t, err := time.Parse(format, value) +// CompatibleParseGCTime parses a string with `GcTimeFormat` and returns a time.Time. If `value` can't be parsed as that +// format, truncate to last space and try again. This function is only useful when loading times that saved by +// gc_worker. We have changed the format that gc_worker saves time (removed the last field), but when loading times it +// should be compatible with the old format. +func CompatibleParseGCTime(value string) (time.Time, error) { + t, err := time.Parse(GcTimeFormat, value) - i := len(value) - for err != nil && i > 0 { - // Cut from the last space - for i > 0 { - i-- - if value[i] == ' ' { - break - } - } - - t, err = time.Parse(format, value[0:i]) + if err != nil { + // Remove the last field that separated by space + parts := strings.Split(value, " ") + prefix := strings.Join(parts[:len(parts)-1], " ") + t, err = time.Parse(GcTimeFormat, prefix) } if err != nil { - err = errors.Errorf("string \"%v\" doesn't has a prefix that matches format \"%v\"", value, format) + err = errors.Errorf("string \"%v\" doesn't has a prefix that matches format \"%v\"", value, GcTimeFormat) } return t, err } diff --git a/util/misc_test.go b/util/misc_test.go index ad7b90c09d9d1..a9f4dc48b623c 100644 --- a/util/misc_test.go +++ b/util/misc_test.go @@ -71,14 +71,11 @@ func (s *testMiscSuite) TestRunWithRetry(c *C) { c.Assert(cnt, Equals, 1) } -func (s *testMiscSuite) TestParseTimeFromPrefix(c *C) { - format := "20060102-15:04:05 -0700" - +func (s *testMiscSuite) TestCompatibleParseGCTime(c *C) { values := []string{ "20181218-19:53:37 +0800 CST", - "20181218-19:53:37 +0800 MST ", + "20181218-19:53:37 +0800 MST", "20181218-19:53:37 +0800 FOO", - "20181218-19:53:37 +0800 aaa bbb ccc 1 2 3 ", "20181218-19:53:37 +0800 +08", "20181218-19:53:37 +0800", "20181218-19:53:37 +0800 ", @@ -91,6 +88,7 @@ func (s *testMiscSuite) TestParseTimeFromPrefix(c *C) { "foo", "20181218-11:53:37", "20181218-19:53:37 +0800CST", + "20181218-19:53:37 +0800 FOO BAR", "20181218-19:53:37 +0800FOOOOOOO BAR", "20181218-19:53:37 ", } @@ -102,16 +100,16 @@ func (s *testMiscSuite) TestParseTimeFromPrefix(c *C) { c.Assert(err, IsNil) for _, value := range values { - t, err := ParseTimeFromPrefix(format, value) + t, err := CompatibleParseGCTime(value) c.Assert(err, IsNil) c.Assert(t.Equal(expectedTime), Equals, true) - formatted := t.In(beijing).Format(format) + formatted := t.In(beijing).Format(GcTimeFormat) c.Assert(formatted, Equals, expectedTimeFormatted) } for _, value := range invalidValues { - _, err := ParseTimeFromPrefix(format, value) + _, err := CompatibleParseGCTime(value) c.Assert(err, NotNil) } } From 5262c5ee660ddd0adda0db9f6937338bc1b96c59 Mon Sep 17 00:00:00 2001 From: MyonKeminta Date: Wed, 26 Dec 2018 15:46:42 +0800 Subject: [PATCH 4/4] rename GcTimeFormat to GCTimeFormat Signed-off-by: MyonKeminta --- store/tikv/gcworker/gc_worker.go | 2 +- util/misc.go | 12 ++++++------ util/misc_test.go | 2 +- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/store/tikv/gcworker/gc_worker.go b/store/tikv/gcworker/gc_worker.go index 296613c52d283..4561b586f176f 100644 --- a/store/tikv/gcworker/gc_worker.go +++ b/store/tikv/gcworker/gc_worker.go @@ -931,7 +931,7 @@ func (w *GCWorker) saveSafePoint(kv tikv.SafePointKV, key string, t uint64) erro } func (w *GCWorker) saveTime(key string, t time.Time) error { - err := w.saveValueToSysTable(key, t.Format(tidbutil.GcTimeFormat)) + err := w.saveValueToSysTable(key, t.Format(tidbutil.GCTimeFormat)) return errors.Trace(err) } diff --git a/util/misc.go b/util/misc.go index 8da6b35e02630..496ef43ca036f 100644 --- a/util/misc.go +++ b/util/misc.go @@ -27,8 +27,8 @@ const ( DefaultMaxRetries = 30 // RetryInterval indicates retry interval. RetryInterval uint64 = 500 - // GcTimeFormat is the format that gc_worker used to store times. - GcTimeFormat = "20060102-15:04:05 -0700" + // GCTimeFormat is the format that gc_worker used to store times. + GCTimeFormat = "20060102-15:04:05 -0700" ) // RunWithRetry will run the f with backoff and retry. @@ -76,22 +76,22 @@ func WithRecovery(exec func(), recoverFn func(r interface{})) { exec() } -// CompatibleParseGCTime parses a string with `GcTimeFormat` and returns a time.Time. If `value` can't be parsed as that +// CompatibleParseGCTime parses a string with `GCTimeFormat` and returns a time.Time. If `value` can't be parsed as that // format, truncate to last space and try again. This function is only useful when loading times that saved by // gc_worker. We have changed the format that gc_worker saves time (removed the last field), but when loading times it // should be compatible with the old format. func CompatibleParseGCTime(value string) (time.Time, error) { - t, err := time.Parse(GcTimeFormat, value) + t, err := time.Parse(GCTimeFormat, value) if err != nil { // Remove the last field that separated by space parts := strings.Split(value, " ") prefix := strings.Join(parts[:len(parts)-1], " ") - t, err = time.Parse(GcTimeFormat, prefix) + t, err = time.Parse(GCTimeFormat, prefix) } if err != nil { - err = errors.Errorf("string \"%v\" doesn't has a prefix that matches format \"%v\"", value, GcTimeFormat) + err = errors.Errorf("string \"%v\" doesn't has a prefix that matches format \"%v\"", value, GCTimeFormat) } return t, err } diff --git a/util/misc_test.go b/util/misc_test.go index a9f4dc48b623c..1ae47e6e2f6a7 100644 --- a/util/misc_test.go +++ b/util/misc_test.go @@ -104,7 +104,7 @@ func (s *testMiscSuite) TestCompatibleParseGCTime(c *C) { c.Assert(err, IsNil) c.Assert(t.Equal(expectedTime), Equals, true) - formatted := t.In(beijing).Format(GcTimeFormat) + formatted := t.In(beijing).Format(GCTimeFormat) c.Assert(formatted, Equals, expectedTimeFormatted) }