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

executor, store: fixed daylight saving time issue #6823

Merged
merged 33 commits into from Jul 16, 2018

Conversation

@zhexuany
Copy link
Member

commented Jun 12, 2018

Thank you for working on TiDB! Please read TiDB's CONTRIBUTING document BEFORE filing this PR.

What have you changed? (mandatory)

During coprocessor dag task, it first uses timezone name, if non-empty, to get legitimate timezone variable. To achieve this, we need push down such data into tikv which leads to change the logic of building pushdown request. The logic I mentioned mainly resides in executor package.

I change timeZoneOffset to zone and add second return parameter name string. The intentioned of doing this to adopt the convention of time package.

For the same purpose, I change GetTimeZone to Location. As you can see, in time package, timezone was bind to Location.

What are the type of the changes (mandatory)?

  • Bug fix (non-breaking change which fixes an issue)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

How has this PR been tested (mandatory)?

@@ -1098,7 +1099,7 @@ func (b *executorBuilder) buildAnalyzeIndexPushdown(task plan.AnalyzeIndexTask)
Tp: tipb.AnalyzeType_TypeIndex,
StartTs: math.MaxUint64,
Flags: statementContextToFlags(b.ctx.GetSessionVars().StmtCtx),
TimeZoneOffset: timeZoneOffset(b.ctx),

This comment has been minimized.

Copy link
@zhexuany

zhexuany Jun 12, 2018

Author Member

@lamxTyler For analyse executor, do we need timezone name too?

This comment has been minimized.

Copy link
@lamxTyler

lamxTyler Jun 13, 2018

Member

Do not need.

This comment has been minimized.

Copy link
@breeswish

breeswish Jun 29, 2018

Member

@lamxTyler Is offset needed in analyzer?

This comment has been minimized.

Copy link
@zhexuany

zhexuany Jul 2, 2018

Author Member

ping @lamxTyler

This comment has been minimized.

Copy link
@lamxTyler

lamxTyler Jul 6, 2018

Member

Seems not needned.

@zhexuany zhexuany force-pushed the zhexuany:fix_daylight_saving_time_issue branch from 2fcc3a2 to 4a59ac4 Jun 12, 2018
@shenli

This comment has been minimized.

Copy link
Member

commented Jun 12, 2018

  1. DO NOT leave the description empty
  2. Could you move the vendor updating into a separate commit?
@zhexuany

This comment has been minimized.

Copy link
Member Author

commented Jun 12, 2018

@shenli I usually created PR via command-line. That is being said, I will fill info later via website.

For second question, I can certainly do this.

@shenli

This comment has been minimized.

Copy link
Member

commented Jun 14, 2018

Please fix the CI and resolve the conflicts.

@zhexuany zhexuany force-pushed the zhexuany:fix_daylight_saving_time_issue branch 8 times, most recently from 654c0d9 to 61e72f8 Jun 14, 2018
@shenli

This comment has been minimized.

Copy link
Member

commented Jun 19, 2018

Please fix the CI.

@zhexuany zhexuany force-pushed the zhexuany:fix_daylight_saving_time_issue branch from e3d702f to 249bd55 Jun 24, 2018
@zhexuany

This comment has been minimized.

Copy link
Member Author

commented Jun 24, 2018

/run-all-tests

@zhexuany

This comment has been minimized.

Copy link
Member Author

commented Jun 24, 2018

@shenli I was on a vacation break. Sorry for the delay. Now, CI has been fixed. PTAL

@coocood

This comment has been minimized.

Copy link
Member

commented Jun 27, 2018

@zhexuany
The default location name is Local, can tikv handle this?

@zhexuany

This comment has been minimized.

Copy link
Member Author

commented Jun 28, 2018

Already checked with engineer from tikv. TiKV does not need handle this.

@zhexuany

This comment has been minimized.

Copy link
Member Author

commented Jun 28, 2018

time.LoadLocation is super slow because it read zip file from disk. I am diving into this and trying to find an alternative way to improve this.

var LocCache *locCache

func (lm *locCache) getLoc(name string) (*time.Location, error) {
if v, ok := lm.locMap[name]; ok {

This comment has been minimized.

Copy link
@coocood

coocood Jun 28, 2018

Member

Need read lock

This comment has been minimized.

Copy link
@zhexuany

zhexuany Jun 28, 2018

Author Member

Yes. This was already fixed.

zhexuany added 2 commits Jun 28, 2018
@zhexuany zhexuany force-pushed the zhexuany:fix_daylight_saving_time_issue branch from c8e3626 to eb92e87 Jul 13, 2018
zhexuany added 2 commits Jul 13, 2018
@@ -1889,7 +1889,7 @@ func (b *builtinSysDateWithFspSig) evalTime(row types.Row) (d types.Time, isNull
return types.Time{}, isNull, errors.Trace(err)
}

tz := b.ctx.GetSessionVars().GetTimeZone()
tz := b.ctx.GetSessionVars().Location()

This comment has been minimized.

Copy link
@zz-jason

zz-jason Jul 13, 2018

Member

s/tz/loc/?

// GetTimeZone returns the value of time_zone session variable.
func (s *SessionVars) GetTimeZone() *time.Location {
// Location returns the value of time_zone session variable. If it is nil, then return time.Local.
func (s *SessionVars) Location() *time.Location {
loc := s.TimeZone

This comment has been minimized.

Copy link
@zz-jason

zz-jason Jul 13, 2018

Member

We may need to rename s.TimeZone to s.Location or any other proper names.

This comment has been minimized.

Copy link
@zhexuany

zhexuany Jul 13, 2018

Author Member

Location is better and also time package uses the same name.

This comment has been minimized.

Copy link
@zz-jason

zz-jason Jul 13, 2018

Member

Yes, we can do it in another PR, keep this PR focused on the daylight saving time issue.

This comment has been minimized.

Copy link
@zhexuany

zhexuany Jul 13, 2018

Author Member

Already did - -.

This comment has been minimized.

Copy link
@zhexuany

zhexuany Jul 13, 2018

Author Member

I changed to Loc eventually.

This comment has been minimized.

Copy link
@zz-jason

This comment has been minimized.

Copy link
@zhexuany

zhexuany Jul 13, 2018

Author Member

I saw a conflict and to avoid further change I undo my changes and I will file another PR for changing this once this PR gets merged into master.

if err != nil {
_, err1 := se.Execute(ctx, "ROLLBACK")
terror.Log(errors.Trace(err1))
return false, errors.Trace(err)
}
err = w.saveTime(gcLeaderLeaseKey, time.Now().Add(gcWorkerLease), se)
fmt.Println("fuck" + time.Now().String())

This comment has been minimized.

Copy link
@zz-jason

zz-jason Jul 13, 2018

Member

remove debug log.

This comment has been minimized.

Copy link
@breeswish

This comment has been minimized.

Copy link
@breeswish

breeswish Jul 13, 2018

Member

I totally understand your mind!

@zhexuany zhexuany force-pushed the zhexuany:fix_daylight_saving_time_issue branch from 6573b27 to 690c644 Jul 13, 2018
Copy link
Member

left a comment

LGTM

@@ -1272,7 +1275,7 @@ func (b *executorBuilder) buildAnalyzeColumnsPushdown(task plan.AnalyzeColumnsTa
Tp: tipb.AnalyzeType_TypeColumn,
StartTs: math.MaxUint64,
Flags: statementContextToFlags(b.ctx.GetSessionVars().StmtCtx),
TimeZoneOffset: timeZoneOffset(b.ctx),
TimeZoneOffset: offset,

This comment has been minimized.

Copy link
@shenli

shenli Jul 13, 2018

Member

Should we add TimeZoneName in Analyze request? @lamxTyler

This comment has been minimized.

Copy link
@zhexuany

zhexuany Jul 16, 2018

Author Member

Already confirmed with him. No need to consider dst in statistics.

@@ -1938,6 +1938,7 @@ func (s *testSuite) TestTimestampTimeZone(c *C) {
r.Check(testkit.Rows("123381351 1734 2014-03-31 08:57:10 127.0.0.1")) // Cover IndexLookupExec

// For issue https://github.com/pingcap/tidb/issues/3485
tk.MustExec("set time_zone = 'Asia/Shanghai'")

This comment has been minimized.

Copy link
@shenli

shenli Jul 13, 2018

Member

Why should we add this line? Is there any dst issue in the test case?

This comment has been minimized.

Copy link
@zhexuany

zhexuany Jul 16, 2018

Author Member

Please refer to line 1907. It sets timezone with offset -6. If we want to compare datetime correctly, we have to set timezone as Asia/Shanghai.

// Talked with Golang team about whether they can have some forms of cache policy available for programmer,
// they suggests that only programmers knows which one is best for their use case.
// For detail, please refer to: https://github.com/golang/go/issues/26106
type locCache struct {

This comment has been minimized.

Copy link
@shenli

shenli Jul 13, 2018

Member

How about locationCache? locCache looks like lockCache.

This comment has been minimized.

Copy link
@zhexuany

zhexuany Jul 16, 2018

Author Member

loc is generally used everywhere in our codebase. Additionally, it is generally used in time package.

if sc.TimeZone, err = LocCache.getLoc(dagReq.TimeZoneName); err != nil {
return nil, nil, nil, errors.Trace(err)
}
dagReq.TimeZoneName = sc.TimeZone.String()

This comment has been minimized.

Copy link
@shenli

shenli Jul 13, 2018

Member

Why set dagReq.TimeZoneName ?

This comment has been minimized.

Copy link
@zhexuany

zhexuany Jul 16, 2018

Author Member

We set dagReq.TimeZoneOffset here, so do dagReq.TimeZoneName.

@@ -240,7 +240,7 @@ func (w *GCWorker) leaderTick(ctx context.Context) error {
return nil
}

// prepare checks required conditions for starting a GC job. It returns a bool
// prepare checks preconditions for starting a GC job. It returns a bool

This comment has been minimized.

Copy link
@shenli

shenli Jul 16, 2018

Member

Why change this file? Is it related to the dst issue?

This comment has been minimized.

Copy link
@zhexuany

zhexuany Jul 16, 2018

Author Member

No. It has nothing to do with dst issue. I change it mainly becuase GCoworker already has session as its variable so we do not need access such info with passing it as parameter around.

I found this on the way fixing dst, it should be done via another PR. I will be carefully next time.

This comment has been minimized.

Copy link
@shenli
@shenli

This comment has been minimized.

Copy link
Member

commented Jul 16, 2018

LGTM

@shenli

This comment has been minimized.

Copy link
Member

commented Jul 16, 2018

/run-all-tests

@shenli shenli added status/LGT2 and removed status/LGT1 labels Jul 16, 2018
@coocood

This comment has been minimized.

Copy link
Member

commented Jul 16, 2018

@zhexuany Please resolve the conflict.

zhexuany added 3 commits Jul 16, 2018
@zhexuany

This comment has been minimized.

Copy link
Member Author

commented Jul 16, 2018

@coocood conflict was already resolved.

@shenli
shenli approved these changes Jul 16, 2018
@zhexuany zhexuany added status/DNM and removed status/DNM labels Jul 16, 2018
@zhexuany zhexuany merged commit 7c18d24 into pingcap:master Jul 16, 2018
4 checks passed
4 checks passed
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci-tidb/build Jenkins job succeeded.
Details
license/cla Contributor License Agreement is signed.
Details
@zhexuany zhexuany deleted the zhexuany:fix_daylight_saving_time_issue branch Jul 16, 2018
zhexuany added a commit to zhexuany/tidb that referenced this pull request Aug 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.