clientv3: don't halt lease client if there is a lease error #7732

heyitsanthony · 2017-04-13T03:04:26Z

xiang90 · 2017-04-13T04:11:35Z

clientv3/lease.go


 	// KeepAliveOnce renews the lease once. In most of the cases, Keepalive
 	// should be used instead of KeepAliveOnce.
-	KeepAliveOnce(ctx context.Context, id LeaseID) (*LeaseKeepAliveResponse, error)
+	KeepAliveOnce(ctx context.Context, id LeaseID) LeaseKeepAliveResponse


do we really need hide the error from keepaliveonce?

xiang90 · 2017-04-13T04:12:16Z

clientv3/lease.go

+	// KeepAlive keeps the given lease alive forever. If the keepalive response posted to
+	// the channel is not consumed immediately, the lease client will continue sending keep alive requests
+	// to the etcd server at least every second until latest response is consumed.
+	KeepAlive(ctx context.Context, id LeaseID) LeaseKeepAliveChan


this breaks the API. one dirty thing we can do is to leave the error there and always return nil.

Well, how should this break code that relies on the client: keep the interface but silently change the behavior or noticeably change the interface to match the new behavior?

how do people handle the error anyway today? probably with a simple retry the same as what we do internally now, right? if we stop returning the error, nothing is going to change for them. they can throw away the error handling code at their pace.

but, yea, we have to communicate this change. it just means that peoples' code can still compile and probably still work as expected.

even we decide to break the api in this kind of minor way, it probably still not a huge deal either. but we need to communicate this very explicitly.

My instinct here is the most explicit and obvious way to communicate the change is to break compilation.

My instinct here is the most explicit and obvious way to communicate the change is to break compilation.

i am fine with this. but right now, client versioning is tight to the entire etcd versioning. so we cannot easily bump the major version of our client to indicate an API change.

Are there docs on this? There's wire-compatibility breakage and there's source-compatibility breakage. The first is way more serious than the second. If the policy is to have it compile but the underlying behavior may change across minor revisions, I don't think that is a meaningful policy whatsoever. The code will statically pass via compilation, but then will suddenly break when running it. Sure the API is "preserved" but you're being wildly disingenuous about what's being guaranteed.

If this is going by semver policy, then this change will violate minor bump behavior no matter what and will need a compat layer instead.

Are there docs on this?

we do not. and, yes, we should.

If the policy is to have it compile but the underlying behavior may change across minor revisions, I don't think that is a meaningful policy whatsoever.

i agree.

If this is going by semver policy, then this change will violate minor bump behavior no matter what and will need a compat layer instead.

ok. probably the compat layer is the way to go for this release. we need to start thinking about breaking client and server versioning in the next release.

OK, so there's no official documentation or policy on it now so that's why there needs to be a compat layer which will gunk up the client even worse? The compat layer stuff won't fix the initial issue because the client will still be using the old interface. There's API-layer breakage in STM and Elections too for 3.2.

We've already done far worse in terms of semver adherence because k8s. Ugh.

xiang90 · 2017-04-24T21:10:09Z

clientv3/lease.go

+			for l.stopCtx.Err() == nil {
+				err := l.recvKeepAliveLoop()
+				if err == context.Canceled {
+					err = nil


why we do not propagate cancel error to ka.Close?

Simulates old behavior where a context cancelation will cause the lease channel to close without another message. Seems better to return the cancel error here instead of hiding it, though. Updated.

xiang90 · 2017-04-24T21:18:33Z

clientv3/lease.go

+			}()
+
+			for l.stopCtx.Err() == nil {
+				err := l.recvKeepAliveLoop()


should be back-off this retry?

OK, using a time.After unless something more complicated belongs here

xiang90 · 2017-04-24T22:06:28Z

clientv3/lease.go

+	// KeepAlive keeps the given lease alive forever. If the keepalive response posted to
+	// the channel is not consumed immediately, the lease client will continue sending keep alive requests
+	// to the etcd server at least every second until latest response is consumed.
+	KeepAlive(ctx context.Context, id LeaseID) LeaseKeepAliveChan


can we talk about when do we close the chan? and what users should do when the returned chan is closed?

xiang90 · 2017-04-24T22:06:42Z

clientv3/lease.go


 	// KeepAliveOnce renews the lease once. In most of the cases, Keepalive
 	// should be used instead of KeepAliveOnce.
-	KeepAliveOnce(ctx context.Context, id LeaseID) (*LeaseKeepAliveResponse, error)
+	KeepAliveOnce(ctx context.Context, id LeaseID) LeaseKeepAliveResponse


same here. talk a little bit about the closing of the chan?

xiang90 · 2017-04-24T22:52:31Z

LGTM

Fixes etcd-io#7488

This reverts commit fbbc4a4, reversing changes made to f254e38. Fixes etcd-io#7851

Revert "Merge pull request #7732 from heyitsanthony/lease-err-ka"

xiang90 reviewed Apr 13, 2017

View reviewed changes

heyitsanthony force-pushed the lease-err-ka branch 3 times, most recently from 1d45517 to 2c608e3 Compare April 18, 2017 18:15

gyuho added the kind/release-note label Apr 18, 2017

heyitsanthony force-pushed the lease-err-ka branch 2 times, most recently from 71a4525 to 83a11fe Compare April 24, 2017 17:12

xiang90 reviewed Apr 24, 2017

View reviewed changes

heyitsanthony force-pushed the lease-err-ka branch from 83a11fe to 1922164 Compare April 24, 2017 22:46

heyitsanthony force-pushed the lease-err-ka branch 4 times, most recently from b617cbf to 2fd6df9 Compare April 25, 2017 06:47

Anthony Romano added 8 commits April 24, 2017 23:49

clientv3: don't halt lease client if there is a lease error

d5f414f

Fixes etcd-io#7488

concurrency: use new lease interface in session

f224d74

clientv3/integration: use new interfaces in lease tests

f305326

etcdcdtl: use new lease interface

70f0bbe

grpcproxy: use new lease interface

b5384ac

etcd-runner: update to use new lease interface

78afc85

benchmark: use new lease interface

cb8524f

integration: close proxy's lease client

2fd6df9

heyitsanthony merged commit fbbc4a4 into etcd-io:master Apr 25, 2017

heyitsanthony deleted the lease-err-ka branch April 25, 2017 14:06

heyitsanthony pushed a commit to heyitsanthony/etcd that referenced this pull request May 2, 2017

Revert "Merge pull request etcd-io#7732 from heyitsanthony/lease-err-ka"

6dd8fb6

This reverts commit fbbc4a4, reversing changes made to f254e38. Fixes etcd-io#7851

heyitsanthony pushed a commit that referenced this pull request May 3, 2017

Merge pull request #7852 from heyitsanthony/revert-lease-err-ka

24e85b2

Revert "Merge pull request #7732 from heyitsanthony/lease-err-ka"

gyuho removed the kind/need-release-note-v3.3 label Nov 22, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

clientv3: don't halt lease client if there is a lease error #7732

clientv3: don't halt lease client if there is a lease error #7732

heyitsanthony commented Apr 13, 2017

xiang90 Apr 13, 2017

xiang90 Apr 13, 2017

heyitsanthony Apr 13, 2017

xiang90 Apr 13, 2017 •

edited

Loading

xiang90 Apr 13, 2017

heyitsanthony Apr 13, 2017

xiang90 Apr 20, 2017 •

edited

Loading

heyitsanthony Apr 20, 2017

heyitsanthony Apr 20, 2017

xiang90 Apr 20, 2017

heyitsanthony Apr 20, 2017

xiang90 Apr 24, 2017

heyitsanthony Apr 24, 2017

xiang90 Apr 24, 2017

heyitsanthony Apr 24, 2017

xiang90 Apr 24, 2017

xiang90 Apr 24, 2017

xiang90 commented Apr 24, 2017

clientv3: don't halt lease client if there is a lease error #7732

clientv3: don't halt lease client if there is a lease error #7732

Conversation

heyitsanthony commented Apr 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiang90 Apr 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiang90 Apr 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiang90 commented Apr 24, 2017

xiang90 Apr 13, 2017 •

edited

Loading

xiang90 Apr 20, 2017 •

edited

Loading