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

session: support setting tso into session variable "tidb_snapshot" #6749

Merged
merged 11 commits into from
Jun 6, 2018

Conversation

zhexuany
Copy link
Contributor

@zhexuany zhexuany commented Jun 4, 2018

After this PR, set @@tidb_snapshot= '400036290571534337' can be used.

@zhexuany zhexuany requested review from coocood and XuHuaiyu and removed request for coocood June 4, 2018 09:47
@coocood
Copy link
Member

coocood commented Jun 4, 2018

LGTM

@coocood
Copy link
Member

coocood commented Jun 4, 2018

/run-all-tests

types/time.go Outdated
@@ -45,6 +45,10 @@ const (
TimeFormat = "2006-01-02 15:04:05"
// TimeFSPFormat is time format with fractional seconds precision.
TimeFSPFormat = "2006-01-02 15:04:05.000000"

// used for encodinh/decoding tso
physicalShiftBits = 18
Copy link
Member

Choose a reason for hiding this comment

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

There is a constant in store/tikv/oracle/oracle.go. Could we use that one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

DONE

@shenli
Copy link
Member

shenli commented Jun 5, 2018

We need to update the related document.

@zhexuany
Copy link
Contributor Author

zhexuany commented Jun 5, 2018

@shenli PTAL


if tso, err2 := strconv.ParseUint(sVal, 10, 64); err2 == nil {
s.SnapshotTS = tso
return errors.Trace(nil)
Copy link
Member

Choose a reason for hiding this comment

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

return nil

@coocood coocood added request-change status/LGT1 Indicates that a PR has LGTM 1. labels Jun 5, 2018
@zhexuany
Copy link
Contributor Author

zhexuany commented Jun 5, 2018

@shenli PTAL

@@ -48,3 +48,15 @@ func ExtractPhysical(ts uint64) int64 {
func GetPhysical(t time.Time) int64 {
return t.UnixNano() / int64(time.Millisecond)
}

// EncodeTso encodes a millisecond into tso.
func EncodeTso(ts int64) uint64 {
Copy link
Contributor

@zimulala zimulala Jun 5, 2018

Choose a reason for hiding this comment

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

s/Tso/TSO ?

}

// DecodeTso decodes a tso into a golang's time struct.
func DecodeTso(tso uint64) time.Time {
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we use this function? Could we remove it?

t, err := types.ParseTime(s.StmtCtx, sVal, mysql.TypeTimestamp, types.MaxFsp)

Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this useless line.

@@ -175,10 +175,18 @@ func setSnapshotTS(s *SessionVars, sVal string) error {
s.SnapshotTS = 0
return nil
}

if tso, err2 := strconv.ParseUint(sVal, 10, 64); err2 == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

s/err2/err ?

@zhexuany
Copy link
Contributor Author

zhexuany commented Jun 5, 2018

@zimulala All comments are addressed. PTAL

@zhexuany zhexuany force-pushed the support_tso_when_use_tidb_snapshot branch from 7c78f53 to e6e88d1 Compare June 5, 2018 16:45
}

// DecodeTSO decodes a tso into a golang's time struct.
func DecodeTSO(tso uint64) time.Time {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we use this function? Could we remove it?

Copy link
Contributor

@zimulala zimulala left a comment

Choose a reason for hiding this comment

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

LGTM

@zimulala zimulala added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jun 6, 2018
@zimulala
Copy link
Contributor

zimulala commented Jun 6, 2018

/run-all-tests

@zhexuany zhexuany merged commit 7314bdc into pingcap:master Jun 6, 2018
@zhexuany zhexuany deleted the support_tso_when_use_tidb_snapshot branch June 6, 2018 04:47
@zhexuany zhexuany changed the title session: support tso when use tidb_snapshot session: support setting tso into session variable "tidb_snapshot" Jun 11, 2018
@shenli
Copy link
Member

shenli commented Jun 11, 2018

@zhexuany We need to cherry-pick this to the release-2.0 branch.

@zhexuany
Copy link
Contributor Author

confirmed with @WangXiangUSTC, as long as this PR be in part of 2.0.4 release, they are fine with this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants