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.

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

gc_worker: Remove timezone name from the times that are saved in mysql.tidb #8745

Merged
merged 10 commits into from
Dec 27, 2018

Conversation

MyonKeminta
Copy link
Contributor

@MyonKeminta MyonKeminta commented Dec 19, 2018

What problem does this PR solve?

gc_worker saves some times in mysql.tidb. These times are: tikv_gc_safe_point, tikv_gc_last_run_time, tikv_gc_leader_lease.

Now our format is 20060102-15:04:05 -0700 MST. The time zone is specified twice by "-0700" and "MST". This seems to be ok, but unfortunately it caused a problem: in some environments, it can't get a timezone name properly, only to produce something like "20181218-19:53:37 +0800 +08". Then if we try to load it from the table, we will get an error when parsing the string, which says cannot parse "+08" as "MST".

This PR tries to solve this problem by deprecating the last field "MST", but still be able to load the time with the old format.

What is changed and how it works?

A function named ParseTimeFromPrefix was added. It tries to parse a time with specified format from a prefix of a given string, and the string should contain a space that separate the prefix and the rest of the string. So when we use this function to parse the string "20181218-19:53:37 +0800 +08" or "20181218-19:53:37 +0800 CST" with the format "20060102-15:04:05 -0700", we actually ignored the last "+08" or "CST". So it's compatible with old format.

Check List

Tests

  • Unit test

Code changes

  • Has exported function/method change
    • util/misc.go: added function ParseTimeFromPrefix
  • Has persistent data change
    • The format of times in mysql.tidb is changed from 20060102-15:04:05 -0700 MST to 20060102-15:04:05 -0700

Side effects

  • No

Related changes

  • Need to cherry-pick to the release branch

This change is Reviewable

Signed-off-by: MyonKeminta <[email protected]>
Signed-off-by: MyonKeminta <[email protected]>
@MyonKeminta MyonKeminta added type/bugfix This PR fixes a bug. component/GC labels Dec 19, 2018
@MyonKeminta
Copy link
Contributor Author

/run-all-tests
@breeswish @zhexuany PTAL

@zz-jason zz-jason added sig/transaction SIG:Transaction and removed component/GC labels Dec 19, 2018
@zhexuany
Copy link
Contributor

@MyonKeminta Please fix ci

@breezewish
Copy link
Member

LGTM

@MyonKeminta
Copy link
Contributor Author

/run-common-test
/run-integration-common-test
/run-unit-test

@MyonKeminta
Copy link
Contributor Author

Sorry.. I didn't make it to figure out what was wrong with CI...

@MyonKeminta
Copy link
Contributor Author

/run-common-test
/run-integration-common-test
/run-unit-test

@MyonKeminta
Copy link
Contributor Author

/run-common-test
/run-integration-common-test
/run-unit-test

@MyonKeminta
Copy link
Contributor Author

/run-all-tests

@MyonKeminta
Copy link
Contributor Author

/run-unit-test

1 similar comment
@MyonKeminta
Copy link
Contributor Author

/run-unit-test

breezewish
breezewish previously approved these changes Dec 25, 2018
@MyonKeminta MyonKeminta added the status/LGT1 Indicates that a PR has LGTM 1. label Dec 25, 2018
@MyonKeminta
Copy link
Contributor Author

@zhexuany PTAL again thanks!

@MyonKeminta
Copy link
Contributor Author

@disksing PTAL

util/misc.go Outdated Show resolved Hide resolved
@jackysp jackysp dismissed breezewish’s stale review December 25, 2018 07:22

Need 2 more LGTMs.

util/misc.go Outdated Show resolved Hide resolved
if err != nil {
// Remove the last field that separated by space
parts := strings.Split(value, " ")
prefix := strings.Join(parts[:len(parts)-1], " ")
Copy link
Contributor

Choose a reason for hiding this comment

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

Will it out of range?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it should returns at least one parts. You split "" by " " then you should got [""]. You can also see that the test has passed.

Copy link
Contributor

@zhexuany zhexuany left a comment

Choose a reason for hiding this comment

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

LGTM

@MyonKeminta MyonKeminta added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Dec 26, 2018
@MyonKeminta
Copy link
Contributor Author

/run-all-tests

@disksing disksing added status/LGT3 The PR has already had 3 LGTM. and removed status/LGT2 Indicates that a PR has LGTM 2. labels Dec 26, 2018
@MyonKeminta
Copy link
Contributor Author

/run-unit-test

@MyonKeminta
Copy link
Contributor Author

/run-all-tests

@MyonKeminta
Copy link
Contributor Author

/run-all-tests

@MyonKeminta MyonKeminta merged commit ffd9ba6 into pingcap:master Dec 27, 2018
@MyonKeminta MyonKeminta deleted the misono/fix-gc-time branch December 27, 2018 04:16
yu34po pushed a commit to yu34po/tidb that referenced this pull request Jan 2, 2019
XuHuaiyu pushed a commit to XuHuaiyu/tidb that referenced this pull request May 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sig/transaction SIG:Transaction status/LGT3 The PR has already had 3 LGTM. type/bugfix This PR fixes a bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants