*: write system timezone into mysql.tidb in bootstrap stage. #7638
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
zhexuany
force-pushed
the
get_tz_name_from_zoneinfo
branch
2 times, most recently
from
8d368ea
to
de4a579
Compare
zhexuany
force-pushed
the
get_tz_name_from_zoneinfo
branch
from
de4a579
to
ec6a822
Compare
coocood
reviewed
Sep 7, 2018
executor/distsql.go
Outdated
| @@ -139,9 +141,15 @@ func zone(sctx sessionctx.Context) (string, int64) { | |||
| if name == "Local" { | |||
| path, err := filepath.EvalSymlinks("/etc/localtime") | |||
| if err != nil { | |||
| log.Errorln(err) | |||
| return "Sysytem", int64(offset) | |||
There was a problem hiding this comment.
Yes. I have no idea why this happens. - - Already fixed.
breezewish
reviewed
Sep 7, 2018
executor/distsql.go
Outdated
| @@ -117,6 +119,18 @@ func closeAll(objs ...Closeable) error { | |||
| return errors.Trace(err) | |||
| } | |||
|
|
|||
| // GetTZNameFromFileName gets IANA timezone name from zoninfo path. | |||
executor/distsql.go
Outdated
| path, err := filepath.EvalSymlinks("/etc/localtime") | ||
| if err != nil { | ||
| log.Errorln(err) | ||
| return "Sysytem", int64(offset) |
executor/distsql.go
Outdated
| idx := strings.Index(path, substr) | ||
| return string(path[idx+len(substr)+1:]), nil | ||
| } | ||
| return "", errors.New("only support softlink has share/zoneinfo as a suffix" + path) |
There was a problem hiding this comment.
maybe this is not kind of a suffix?
| @@ -164,13 +165,26 @@ func (h *rpcHandler) buildDAGExecutor(req *coprocessor.Request) (*dagContext, ex | |||
| return ctx, e, dagReq, err | |||
| } | |||
|
|
|||
| func getTZNameFromFileName(path string) (string, error) { | |||
There was a problem hiding this comment.
why is it implemented again here?
breezewish
reviewed
Sep 7, 2018
executor/distsql.go
Outdated
| @@ -127,7 +141,17 @@ func zone(sctx sessionctx.Context) (string, int64) { | |||
| var name string | |||
| name = loc.String() | |||
| if name == "Local" { | |||
| name = "System" | |||
| path, err := filepath.EvalSymlinks("/etc/localtime") | |||
There was a problem hiding this comment.
How is this function evaluated? If it is evaluated frequently, we need to cache the zone.
lysu
reviewed
Sep 7, 2018
executor/distsql.go
Outdated
| idx := strings.Index(path, substr) | ||
| return string(path[idx+len(substr)+1:]), nil | ||
| } | ||
| return "", errors.New("only support softlink has share/zoneinfo as a suffix" + path) |
| @@ -164,13 +165,26 @@ func (h *rpcHandler) buildDAGExecutor(req *coprocessor.Request) (*dagContext, ex | |||
| return ctx, e, dagReq, err | |||
| } | |||
|
|
|||
| func getTZNameFromFileName(path string) (string, error) { | |||
There was a problem hiding this comment.
seems be duplicated to GetTZNameFromFileName and no any caller~?
breezewish
reviewed
Sep 7, 2018
breezewish
previously approved these changes
Sep 7, 2018
|
@lysu PTAL |
breezewish
previously approved these changes
Sep 8, 2018
|
LGTM |
|
/run-all-tests |
…into get_tz_name_from_zoneinfo
|
/run-all-tests |
tiancaiamao
reviewed
Sep 14, 2018
util/timeutil/time.go
Outdated
|
|
||
| // Local returns time.Local's IANA timezone name. It is TiDB's global timezone name. | ||
| func Local() *time.Location { | ||
| loc, err := LoadLocation(systemTZ) |
There was a problem hiding this comment.
What happen when Local() is called before or during cluster bootstrapping ?
systemTZ is not initialized yet.
There was a problem hiding this comment.
see line 11. I assign "System" to it in order to avoid test case failure.
zhexuany
force-pushed
the
get_tz_name_from_zoneinfo
branch
from
1389abc
to
bd1693e
Compare
|
@shenli @tiancaiamao @breeswish PTAL |
|
/run-all-tests |
shenli
reviewed
Sep 14, 2018
session/session.go
Outdated
| func loadSystemTZ(se *session) (string, error) { | ||
| sql := `select variable_value from mysql.tidb where variable_name = "system_tz"` | ||
| rss, errLoad := se.Execute(context.Background(), sql) | ||
| defer rss[0].Close() |
There was a problem hiding this comment.
If errLoad is not nill, the rss maybe empty.
|
@shenli @tiancaiamao PTAL |
shenli
reviewed
Sep 14, 2018
zhexuany
force-pushed
the
get_tz_name_from_zoneinfo
branch
2 times, most recently
from
2212e63
to
fb8584d
Compare
zhexuany
force-pushed
the
get_tz_name_from_zoneinfo
branch
from
fb8584d
to
64a5b2e
Compare
shenli
reviewed
Sep 14, 2018
util/timeutil/time.go
Outdated
| } | ||
|
|
||
| // getLoc first trying to load location from a cache map. If nothing found in such map, then call | ||
| // `time.LoadLocation` to get a timezone location. After trying both way, an error wil be returned |
util/timeutil/time.go
Outdated
| // `time.LoadLocation` to get a timezone location. After trying both way, an error wil be returned | ||
| // if valid Location is not found. | ||
| func (lm *locCache) getLoc(name string) (*time.Location, error) { | ||
| if name == "System" { |
There was a problem hiding this comment.
Define a constant for "System". Should we consider case insensitive comparison?
| name = loc.String() | ||
| // when we found name is "SystemLocation", we have no chice but push down | ||
| // "System" to tikv side. | ||
| if name == "Local" { |
There was a problem hiding this comment.
Please make sure TiKV would handle this @breeswish
|
LGTM @shenli |
|
/run-all-tests |
shenli
approved these changes
Sep 14, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
It retrieves real IANA timezone name when the location is local. This can avoid the performance delegation on TiKV side.
What is changed and how it works?
We need first get the soft link from
/etc/localtime. And then check whether the soft link path containsshare/zoneinfoor not. If yes, we get the real IANA timezone name from this path.If you want to know more about how this PR works please refer to this PR
Check List
Tests
Code changes
Side effects
Related changes