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

*: disable v2 API by default #10935

Merged
merged 4 commits into from
Jul 29, 2019
Merged

*: disable v2 API by default #10935

merged 4 commits into from
Jul 29, 2019

Conversation

gyuho
Copy link
Contributor

@gyuho gyuho commented Jul 25, 2019

Still need changelog.

/cc @xiang90 @jpbetz @philips

@philips
Copy link
Contributor

philips commented Jul 25, 2019

The motivation for this is to disable it by default in v3.4?

@gyuho
Copy link
Contributor Author

gyuho commented Jul 25, 2019

@philips Yes. We will still keep v2 storage and v2 API, until we have v4.

@gyuho gyuho added the WIP label Jul 25, 2019
@philips
Copy link
Contributor

philips commented Jul 25, 2019

OK, this will need major documentation, changelog, and user outreach. Has any of that been planned on an issue?

@gyuho
Copy link
Contributor Author

gyuho commented Jul 25, 2019

Yeah I will make sure we highlight this in our documentation as a breaking change.

@gyuho gyuho removed the WIP label Jul 25, 2019
@codecov-io
Copy link

codecov-io commented Jul 25, 2019

Codecov Report

Merging #10935 into master will decrease coverage by 0.12%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10935      +/-   ##
==========================================
- Coverage   63.93%    63.8%   -0.13%     
==========================================
  Files         400      400              
  Lines       37415    37415              
==========================================
- Hits        23920    23874      -46     
- Misses      11866    11935      +69     
+ Partials     1629     1606      -23
Impacted Files Coverage Δ
etcdserver/server.go 68.48% <ø> (+0.26%) ⬆️
embed/config.go 50.7% <ø> (ø) ⬆️
auth/store.go 45.08% <0%> (-17.76%) ⬇️
proxy/grpcproxy/register.go 69.44% <0%> (-11.12%) ⬇️
clientv3/retry_interceptor.go 65% <0%> (-4.5%) ⬇️
raft/tracker/inflights.go 91.83% <0%> (-4.09%) ⬇️
clientv3/leasing/txn.go 88.09% <0%> (-3.18%) ⬇️
clientv3/concurrency/election.go 79.68% <0%> (-2.35%) ⬇️
clientv3/balancer/balancer.go 86.36% <0%> (-2.28%) ⬇️
etcdserver/api/v2http/client.go 84.3% <0%> (-1.21%) ⬇️
... and 25 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 89d4002...2f30e9a. Read the comment docs.

@jingyih
Copy link
Contributor

jingyih commented Jul 25, 2019

FYI. Noticed some broken unit test:

https://travis-ci.com/etcd-io/etcd/jobs/219715780#L585

@gyuho gyuho mentioned this pull request Jul 26, 2019
19 tasks
@gyuho
Copy link
Contributor Author

gyuho commented Jul 29, 2019

I checked the code base, and confirmed that publish method won't be affected by this. It goes directly to rafthttp. So even with v2 client handler disabled (via --enable-v2=false), cluster can still process publish requests (which encodes requests using v2 storage). Added a TODO to replace this later with v3 storage.

@@ -1980,6 +1980,12 @@ func (s *EtcdServer) sync(timeout time.Duration) {
// static clientURLs of the server.
// The function keeps attempting to register until it succeeds,
// or its server is stopped.
//
// Use v2 store to encode member attributes, and apply through Raft
Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed.

@gyuho
Copy link
Contributor Author

gyuho commented Jul 29, 2019

@jingyih Now CI should be passing. PTAL. Thx.

@jingyih
Copy link
Contributor

jingyih commented Jul 29, 2019

LGTM

@gyuho gyuho merged commit 324952c into etcd-io:master Jul 29, 2019
@gyuho gyuho deleted the v2 branch July 29, 2019 22:43
@jingyih
Copy link
Contributor

jingyih commented Aug 13, 2019

@gyuho I did not see a corresponding changelog for this?

@gyuho
Copy link
Contributor Author

gyuho commented Aug 13, 2019

@jingyih Ah, totally missed that. Let me add now. Thanks.

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

Successfully merging this pull request may close these issues.

4 participants