Add error return to the Registrar interface functions #782
Conversation
The Register and Deregister method are logging errors but not returning any error, which makes error handling not possible. The implementation of the error is meant to reflect error from the client api, not from what we consider an error (re-registering, ...).
| r.logger.Log("err", err) | ||
| } else { | ||
| r.logger.Log("action", "register") | ||
| } | ||
| if r.service.TTL != nil { | ||
| go r.loop() | ||
| } | ||
| return err |
There was a problem hiding this comment.
If Register returns an error, it shouldn't do other things, like go r.loop, don't you think?
There was a problem hiding this comment.
Actually the loop tries to register every TTL, so I feel that I still should let the loop be launched. I don't know how this should be handled.
| p.logger.Log("err", err) | ||
| } else { | ||
| p.logger.Log("action", "register") | ||
| return err |
There was a problem hiding this comment.
Don't log an error you also return.
| p.logger.Log("err", err) | ||
| } else { | ||
| p.logger.Log("action", "deregister") | ||
| return err |
There was a problem hiding this comment.
Don't log an error you also return.
| r.quitmtx.Lock() | ||
| defer r.quitmtx.Unlock() | ||
| if r.quit != nil { | ||
| close(r.quit) | ||
| r.quit = nil | ||
| } | ||
| return err |
There was a problem hiding this comment.
Same thing here, if Deregister returns an error, it shouldn't do other stuff.
I think this reveals a fundamental design tension: what should an application do if its service discovery Register or Deregister action fails? Can it do anything useful with that error besides log it? Arguably, Register failure might want to terminate the service, in some circumstances, but if Deregister fails, can/should we do anything?
| if err := r.client.Register(r.service); err != nil { | ||
| r.logger.Log("err", err) | ||
| return | ||
| return err |
| @@ -93,4 +95,5 @@ func (r *Registrar) Deregister() { | |||
| close(r.quit) | |||
| r.quit = nil | |||
| } | |||
| return err | |||
There was a problem hiding this comment.
Similar questions about Deregister as above.
|
Hey thanks for the review and the very pertinent comments! The reason for returning an error is exactly the use-case you pointed out.
I would prefer having an error returned and do something about it (or just not using it), than having nothing and try to find out somehow if my action succeeded. The log/return thing was let like this because I don't wanted to change the actual logging pattern, but I'll change this as soon as I have time to. |
|
Until now we've taken the approach that an error in Register or Deregister isn't fatal, and that we should log it, but keep doing whatever it is we're doing in the background, in the hopes that it eventually resolves. I understand that this PR supposes it would be nice to let the application know when these things fail, and in the abstract I agree, but we've discovered that changing the semantics of errors in these methods is more subtle than we anticipated. By convention, a non-nil error means the function failed, and any other value returned by the it is undefined; I don't think we can break that convention. Should we have another "channel" (not necessarily a Go chan) on which to report errors to the application? Is that added complexity in every constructor, or Register/Deregister call, worth the value it brings? In case of error, should we keep any registration loop goroutines running, or terminate them? I don't have good answers to these questions. I could be convinced there are good answers to them, but I don't think what we have right now is it. |
|
I think the first issue here is that there are 2 registration workflow present here. The (1) first one is a simple service register where it will hold the service until a potential healthcheck fail for a defined amount of time, or it simply un-register. The (2) second one is a an insert in the k/v store with a TTL, that needs a loop to be kept present.
|
|
I agree that the Register and Deregister methods don't return any errors it's kind strange and @jstoja proposal makes sense. The problem is that different implementations follow different ways. In case of consul it's easy - we can just return error and that's it. If to speak about eureka it's more complicated: uses loops, heartbeats and so on.
|
The Register and Deregister method are logging errors but not
returning any error, which makes error handling not possible.
The implementation of the error is meant to reflect error from the
client api, not from what we consider an error (re-registering, ...).
Closes #780.