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

Replace ugorji/codec with json-iterator/go #10667

Merged
merged 6 commits into from
Apr 23, 2019

Conversation

dims
Copy link
Contributor

@dims dims commented Apr 22, 2019

Kubernetes removed the usage for ugorji in the following PR:
Use json-iterator instead of ugorji for JSON. #48287

However since etcd still uses this library, we end up increasing the size of the binaries in kubernetes by a lot. See:
kubernetes/kubernetes#76506

Can we please switch over to json-iterator/go as well to eliminate the unnecessary code/size?

Change-Id: Ia8b267e9ada49b22a1cd3bd7c05450e32582dc91

Please read https://github.com/etcd-io/etcd/blob/master/CONTRIBUTING.md#contribution-flow.

@dims dims force-pushed the replace-ugorji-with-json-iterator branch 3 times, most recently from 95d7354 to 09aebaf Compare April 22, 2019 23:30
@dims dims changed the title [WIP] Replace ugorji/codec with json-iterator/go Replace ugorji/codec with json-iterator/go Apr 23, 2019
@dims
Copy link
Contributor Author

dims commented Apr 23, 2019

@gyuho @spzala any idea how i can get the amd64-e2e of the semaphoreci to pass?

@spzala
Copy link
Member

spzala commented Apr 23, 2019

@dims I don't see any failure related to your changes looking at quickly. Lately, CI is flakey. I will be reviewing the changes too. Thanks!

@dims dims force-pushed the replace-ugorji-with-json-iterator branch from 8f7131f to 4514211 Compare April 23, 2019 14:55
@liggitt
Copy link
Contributor

liggitt commented Apr 23, 2019

github.com/ugorji/go is still listed in go.sum and is vendored... did you run go mod tidy && go mod vendor?

@dims
Copy link
Contributor Author

dims commented Apr 23, 2019

@liggitt unfortunately the scripts/updatedep.sh does not seem to work well, it just nukes everything in go.mod and go.sum. So i am hesitating to add a commit with the cleanup

@codecov-io
Copy link

codecov-io commented Apr 23, 2019

Codecov Report

Merging #10667 into master will decrease coverage by 0.11%.
The diff coverage is 40%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10667      +/-   ##
==========================================
- Coverage   71.63%   71.51%   -0.12%     
==========================================
  Files         393      394       +1     
  Lines       36629    36658      +29     
==========================================
- Hits        26238    26217      -21     
- Misses       8551     8590      +39     
- Partials     1840     1851      +11
Impacted Files Coverage Δ
client/keys.go 91.45% <100%> (ø) ⬆️
client/json.go 37.93% <37.93%> (ø)
pkg/tlsutil/tlsutil.go 86.2% <0%> (-6.9%) ⬇️
pkg/fileutil/purge.go 65.9% <0%> (-6.82%) ⬇️
pkg/netutil/netutil.go 63.11% <0%> (-6.56%) ⬇️
etcdserver/api/v3rpc/lease.go 71.59% <0%> (-5.69%) ⬇️
pkg/logutil/zap_grpc.go 47.61% <0%> (-4.77%) ⬇️
proxy/grpcproxy/watcher.go 89.79% <0%> (-4.09%) ⬇️
pkg/testutil/recorder.go 77.77% <0%> (-3.71%) ⬇️
clientv3/leasing/txn.go 88.09% <0%> (-3.18%) ⬇️
... and 18 more

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 216808e...3110467. Read the comment docs.

@dims
Copy link
Contributor Author

dims commented Apr 23, 2019

@liggitt cleaned up as well! dropped 45k+ LOC

@gyuho
Copy link
Contributor

gyuho commented Apr 23, 2019

@dims Could you update the commit title to Expected commit title format '<package>{", "<package>}: <description>'?

client/json.go Outdated Show resolved Hide resolved
client/keys.go Outdated Show resolved Hide resolved
@dims
Copy link
Contributor Author

dims commented Apr 23, 2019

@gyuho ack will fix the commit message shortly

@dims dims force-pushed the replace-ugorji-with-json-iterator branch from 3110467 to b7b212d Compare April 23, 2019 18:54
We need to use the stdlib-compatible one that is case-sensitive, etc

Change-Id: Id0df573a70e09967ac7d8c0a63d99d6a49ce82f1
Change-Id: Iac4601443bcad71920fd96b97bfe21c16116577a
Change-Id: I1f3fc00f95efadd6da9b4c248156f8460ae0ff97
Change-Id: Ibfa24e28cacd58388f7606a945c8ac35e1c34580
…ugorji

Using lessons learned from k8s changes:
kubernetes/kubernetes#65034

Change-Id: Ia17a8f94ae6ed00c5af2595c2b48d3c9a0344427
@dims dims force-pushed the replace-ugorji-with-json-iterator branch from b7b212d to 3655a4b Compare April 23, 2019 20:55
@dims
Copy link
Contributor Author

dims commented Apr 23, 2019

@gyuho @spzala would you consider a backport to release-3.3 branch? (looks like k/k is using v3.3.10+ currently)

@gyuho
Copy link
Contributor

gyuho commented Apr 23, 2019

@dims Sure, if it doesn't break compatibility. Would anything change in json-iterator/go?

@spzala
Copy link
Member

spzala commented Apr 23, 2019

@dims aha..all green checks, looks good :-)

@gyuho gyuho merged commit 1697c06 into etcd-io:master Apr 23, 2019
@dims
Copy link
Contributor Author

dims commented Apr 23, 2019

@gyuho exact same code in this PR applies cleanly to 3.3-release (except for the vanity url). So we should be good

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

5 participants