server/api/admin: change the tso parameter of ResetTS to string #1896

5kbpers · 2019-11-05T08:51:23Z

Signed-off-by: 5kbpers tangminghua@pingcap.com

What problem does this PR solve?

the JSON lib can only marshall number to float64 type, which may lose precision when converting the tso parameter from uint64 to float64.

What is changed and how it works?

This PR changes the tso parameter of ResetTS to string to avoid precision losing.

Check List

Tests

Unit test

Code changes

Has HTTP API interfaces change

Signed-off-by: 5kbpers <tangminghua@pingcap.com>

CLAassistant · 2019-11-05T08:51:51Z

All committers have signed the CLA.

server/api/admin_test.go

lhy1024

LGTM. except CI.

Signed-off-by: 5kbpers <tangminghua@pingcap.com>

codecov-io · 2019-11-06T02:22:39Z

Codecov Report

Merging #1896 into master will increase coverage by 0.04%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1896      +/-   ##
==========================================
+ Coverage   77.79%   77.83%   +0.04%     
==========================================
  Files         163      163              
  Lines       16333    16337       +4     
==========================================
+ Hits        12706    12716      +10     
+ Misses       2617     2614       -3     
+ Partials     1010     1007       -3

Impacted Files	Coverage Δ
server/api/admin.go	`69.44% <100%> (+3.81%)`	⬆️
pkg/testutil/operator_check.go	`88.88% <0%> (-11.12%)`	⬇️
server/region_syncer/client.go	`82.89% <0%> (-2.64%)`	⬇️
pkg/btree/btree.go	`86.94% <0%> (+0.8%)`	⬆️
server/member/leader.go	`78.57% <0%> (+3.06%)`	⬆️
pkg/etcdutil/etcdutil.go	`88.4% <0%> (+5.79%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 37efcb0...738a00f. Read the comment docs.

nolouch · 2019-11-06T03:15:01Z

/merge

sre-bot · 2019-11-06T03:17:12Z

/run-all-tests

sre-bot · 2019-11-06T03:21:10Z

@5kbpers merge failed.

5kbpers · 2019-11-06T03:22:01Z

/run-integration-common-test

server/api/admin_test.go

Signed-off-by: 5kbpers <tangminghua@pingcap.com>

shafreeck · 2019-11-06T07:12:45Z

server/api/admin.go

@@ -59,13 +59,18 @@ func (h *adminHandler) ResetTS(w http.ResponseWriter, r *http.Request) {
 	if err := apiutil.ReadJSONRespondError(h.rd, w, r.Body, &input); err != nil {
 		return
 	}
-	ts, ok := input["tso"].(float64)
-	if !ok || ts < 0 {
+	tsValue, ok := input["tso"].(string)


Why not use uint64as the type?

There is no such thing in js spec. 😕

Considering this: https://play.golang.org/p/8-COmwYna7T

I think it is supported in go.

I think it also works by defining var input map[string]uint64.

@5kbpers cc

I'm afraid of it does not work in other languages, eg, rust, as it's not specified in json spec.

@overvenus The language does not matter much, maybe the encoding library does. However, there should be no precision loss when encoding a number because that JSON is based on text not binary.

If you encode a large number into JSON, it is just a sequence of digitals. IMO there is no reason for an encoding library to lose precision.

server/api/admin_test.go

Signed-off-by: 5kbpers <tangminghua@pingcap.com>

nolouch · 2019-11-07T05:35:04Z

PTAL @shafreeck @overvenus

sre-bot · 2019-11-07T06:18:08Z

/run-all-tests

sre-bot · 2019-11-07T06:27:59Z

cherry pick to release-3.1 in PR #1909

… (#1909) Signed-off-by: 5kbpers <tangminghua@pingcap.com>

server/api/admin: change tso parameter of ResetTS to string

7a12136

Signed-off-by: 5kbpers <tangminghua@pingcap.com>

5kbpers requested a review from nolouch November 5, 2019 08:51

5kbpers requested a review from rleungx November 5, 2019 08:51

5kbpers mentioned this pull request Nov 5, 2019

restore: reset ts before starting to restore pingcap/br#31

Merged

nolouch approved these changes Nov 5, 2019

View reviewed changes

nolouch requested changes Nov 5, 2019

View reviewed changes

server/api/admin_test.go Show resolved Hide resolved

lhy1024 approved these changes Nov 5, 2019

View reviewed changes

fix test

38c8457

Signed-off-by: 5kbpers <tangminghua@pingcap.com>

nolouch approved these changes Nov 6, 2019

View reviewed changes

sre-bot added the status/can-merge Indicates a PR has been approved by a committer. label Nov 6, 2019

Merge branch 'master' into reset-ts-parameter

8264591

5kbpers added the needs-cherry-pick-release-3.1 The PR needs to cherry pick to release-3.1 branch. label Nov 6, 2019

rleungx reviewed Nov 6, 2019

View reviewed changes

server/api/admin_test.go Outdated Show resolved Hide resolved

address comments

641fe42

Signed-off-by: 5kbpers <tangminghua@pingcap.com>

5kbpers force-pushed the reset-ts-parameter branch from 17721c1 to 641fe42 Compare November 6, 2019 06:05

shafreeck reviewed Nov 6, 2019

View reviewed changes

overvenus closed this Nov 6, 2019

overvenus reopened this Nov 6, 2019

overvenus reviewed Nov 6, 2019

View reviewed changes

server/api/admin_test.go Outdated Show resolved Hide resolved

shafreeck removed the status/can-merge Indicates a PR has been approved by a committer. label Nov 6, 2019

address comments

738a00f

Signed-off-by: 5kbpers <tangminghua@pingcap.com>

Merge branch 'master' into reset-ts-parameter

1347dae

overvenus added the status/can-merge Indicates a PR has been approved by a committer. label Nov 7, 2019

sre-bot merged commit d96c622 into tikv:master Nov 7, 2019

sre-bot mentioned this pull request Nov 7, 2019

server/api/admin: change the tso parameter of ResetTS to string (#1896) #1909

Merged

overvenus pushed a commit that referenced this pull request Nov 7, 2019

server/api/admin: change the tso parameter of ResetTS to string (#1896)…

472c2e0

… (#1909) Signed-off-by: 5kbpers <tangminghua@pingcap.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

server/api/admin: change the tso parameter of ResetTS to string #1896

server/api/admin: change the tso parameter of ResetTS to string #1896

5kbpers commented Nov 5, 2019

CLAassistant commented Nov 5, 2019 •

edited

lhy1024 left a comment

codecov-io commented Nov 6, 2019 •

edited

nolouch commented Nov 6, 2019

sre-bot commented Nov 6, 2019

sre-bot commented Nov 6, 2019

5kbpers commented Nov 6, 2019

shafreeck Nov 6, 2019

overvenus Nov 6, 2019

shafreeck Nov 6, 2019

shafreeck Nov 6, 2019 •

edited

nolouch Nov 6, 2019

overvenus Nov 6, 2019

shafreeck Nov 6, 2019

shafreeck Nov 6, 2019

nolouch commented Nov 7, 2019

sre-bot commented Nov 7, 2019

sre-bot commented Nov 7, 2019

server/api/admin: change the tso parameter of ResetTS to string #1896

server/api/admin: change the tso parameter of ResetTS to string #1896

Conversation

5kbpers commented Nov 5, 2019

What problem does this PR solve?

What is changed and how it works?

Check List

CLAassistant commented Nov 5, 2019 • edited

lhy1024 left a comment

Choose a reason for hiding this comment

codecov-io commented Nov 6, 2019 • edited

Codecov Report

nolouch commented Nov 6, 2019

sre-bot commented Nov 6, 2019

sre-bot commented Nov 6, 2019

5kbpers commented Nov 6, 2019

shafreeck Nov 6, 2019

Choose a reason for hiding this comment

overvenus Nov 6, 2019

Choose a reason for hiding this comment

shafreeck Nov 6, 2019

Choose a reason for hiding this comment

shafreeck Nov 6, 2019 • edited

Choose a reason for hiding this comment

nolouch Nov 6, 2019

Choose a reason for hiding this comment

overvenus Nov 6, 2019

Choose a reason for hiding this comment

shafreeck Nov 6, 2019

Choose a reason for hiding this comment

shafreeck Nov 6, 2019

Choose a reason for hiding this comment

nolouch commented Nov 7, 2019

sre-bot commented Nov 7, 2019

sre-bot commented Nov 7, 2019

CLAassistant commented Nov 5, 2019 •

edited

codecov-io commented Nov 6, 2019 •

edited

shafreeck Nov 6, 2019 •

edited