Add TTL for etcd sd#413
Conversation
peterbourgon
left a comment
peterbourgon
left a comment
There was a problem hiding this comment.
Hi, thanks for the submission, and I'm sorry it took me so long—I overlooked the notification when it came originally! This will be a welcome addition. There are several things that need attention, please let me know if any of them are unclear.
sd/etcd/registrar.go
Outdated
| etcd "github.com/coreos/etcd/client" | ||
|
|
||
| "github.com/go-kit/kit/log" | ||
| "time" |
There was a problem hiding this comment.
This should be grouped with the other stdlib imports.
|
|
||
| const ( | ||
| MinHeartBeatTime = time.Millisecond * 500 | ||
| ) |
There was a problem hiding this comment.
This constant should either be made private (I think that's preferable) or given a doc comment.
sd/etcd/registrar.go
Outdated
| DeleteOptions *etcd.DeleteOptions | ||
| } | ||
|
|
||
| // TTLOption allow setting a key with a TTL, and regularly refreshes the lease with a goroutine |
There was a problem hiding this comment.
This comment describes the mechanics of what happens, but not what benefit it provides. Can you amend the comment to tell the user in which circumstances they would want to use this option? Also, please wrap the comment at 80 columns, and use proper punctuation (i.e. add a period to the end of the sentence).
| type TTLOption struct { | ||
| Heartbeat time.Duration | ||
| TTL time.Duration | ||
| } |
There was a problem hiding this comment.
Given that a TTLOption is constructed with a constructor, I don't think either of these fields needs to be exported. Is that correct?
| } | ||
|
|
||
| // NewTTLOption returns a TTLOption | ||
| func NewTTLOption(heartbeat, ttl time.Duration) *TTLOption { |
There was a problem hiding this comment.
The doc comment here is a little sparse. Could it please describe what each of the parameters are, and some good default values? It would also be good to describe the bounds-checking logic implemented in the function body.
sd/etcd/registrar.go
Outdated
| } | ||
| } | ||
| } | ||
| }() |
There was a problem hiding this comment.
Please extract this to a loop method on the registrar type.
sd/etcd/registrar.go
Outdated
| select { | ||
| case <-r.quit: | ||
| return | ||
| case <-time.After(r.service.TTL.Heartbeat): |
There was a problem hiding this comment.
This will generate a lot of unnecessary garbage. It's better to use a time.Ticker, constructed outside of the for loop.
| if r.service.TTL == nil { | ||
| return | ||
| } | ||
| r.quit = make(chan struct{}) |
There was a problem hiding this comment.
This mutates registrar state, which makes it unsafe for concurrent access from multiple goroutines. I think the registrar needs its internal state protected with a mutex, which should be taken with every public method.
| } | ||
| if r.quit != nil { | ||
| close(r.quit) | ||
| } |
There was a problem hiding this comment.
It looks like r.quit should also be reset to nil.
| return err | ||
| } | ||
| _, err := c.keysAPI.Create(c.ctx, s.Key, s.Value) | ||
| return err |
There was a problem hiding this comment.
Hmm. What do you think about
var err error
if s.TTL != nil {
_, err = c.keysAPI.Set(...)
} else {
_, err = c.keysAPI.Create(...)
}
return err
|
Thank you for your attention and advice. I have fixed them. |
|
Amazing, thank you! There are a couple of other small things, but I'll make the changes in master after the merge. |
Add TTL for etcd sd
2 participants
Add TTL for registering keys to etcd, with backward compatibility。
relevant issue: #343
Users can set TTL options like this: