-
Notifications
You must be signed in to change notification settings - Fork 9.8k
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
session: remove session manager and add ttl #6172
Conversation
/cc @heyitsanthony |
@@ -39,17 +39,14 @@ type Election struct { | |||
} | |||
|
|||
// NewElection returns a new election on a given key prefix. | |||
func NewElection(client *v3.Client, pfx string) *Election { | |||
func NewElection(client *ClientWithSession, pfx string) *Election { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@heyitsanthony Is this what you want? In your example, election only takes session as arg. But it is not enough since we still need to create keys by using the client for example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@xiang90 Just NewElection(s *Session, pfx string)
. The Session
can have a function func (s *Session) Client() *clientv3.Client { return s.client}
for anything that needs to access etcd through the session. The ClientWithSession
struct probably doesn't need to be part of the concurrency package.
if ttl <= 0 { | ||
so.ttl = defaultSessionTTL | ||
} | ||
so.ttl = ttl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move above if
?
lgtm aside from broken ttl stuff |
f21712d
to
9a53d2c
Compare
Add TTL support, and also remove the session manager.
The plan is to have a sessionClient that embeds a client with its session. So the client can inspect its session more easily.