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

etcdserver: include IsLeader in etcdserverpb.Member #4487

Merged
merged 2 commits into from
Feb 11, 2016

Conversation

gyuho
Copy link
Contributor

@gyuho gyuho commented Feb 11, 2016

This is especially useful to functional-test leader failure.

For #4477.

@gyuho
Copy link
Contributor Author

gyuho commented Feb 11, 2016

/cc @xiang90 @heyitsanthony

Thanks.

@heyitsanthony
Copy link
Contributor

would this be better as a flag in message Member instead of having a dedicated function?

membs := cs.cluster.Members()
var peerURLs []string
if len(membs) > 0 {
peerURLs = membs[0].PeerURLs
Copy link
Contributor

Choose a reason for hiding this comment

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

is membs[0] always the leader?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's sorted by ID, so I think it's not always (https://github.com/coreos/etcd/blob/master/etcdserver/member.go#L160-L165).

@gyuho
Copy link
Contributor Author

gyuho commented Feb 11, 2016

@heyitsanthony I will try to include leader information in Member message and let get leader with MemberList method, instead of having this function.

Thanks.

@gyuho
Copy link
Contributor Author

gyuho commented Feb 11, 2016

@heyitsanthony Just addressed. PTAL.Thanks!

@@ -323,10 +323,11 @@ message Member {
uint64 ID = 1;
// If the member is not started, name will be an empty string.
string name = 2;
repeated string peerURLs = 3;
uint64 LeaderID = 3;
Copy link
Contributor

Choose a reason for hiding this comment

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

bool isLeader?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah boolean is easier to use. Will change.

@gyuho gyuho changed the title etcdserver: add MemberLeader API etcdserver: include IsLeader in etcdserverpb.Member Feb 11, 2016
@gyuho
Copy link
Contributor Author

gyuho commented Feb 11, 2016

@heyitsanthony PTAL. Just changed it to IsLeader. Thanks!

@xiang90
Copy link
Contributor

xiang90 commented Feb 11, 2016

can we add another API called memberLeader instead of adding this field?

@xiang90
Copy link
Contributor

xiang90 commented Feb 11, 2016

Or shall we do it in client library?

@gyuho
Copy link
Contributor Author

gyuho commented Feb 11, 2016

@xiang90 I have code that adds MemberLeader but I think just having boolean value in Member and get it from MemberList feel simpler.

@heyitsanthony
Copy link
Contributor

@xiang90 MemberLeader() can go in the client library.

@gyuho
Copy link
Contributor Author

gyuho commented Feb 11, 2016

So MemberLeader in clientv3? Think it makes sense since we do it in v2 client.

@heyitsanthony
Copy link
Contributor

yeah, the clientv3 implementation would just process memberlist if it's using the bool flag

@gyuho
Copy link
Contributor Author

gyuho commented Feb 11, 2016

Ok will add that. Thanks.

@gyuho
Copy link
Contributor Author

gyuho commented Feb 11, 2016

Just added MemberLeader method. PTAL. Thanks!

@heyitsanthony
Copy link
Contributor

lgtm

@gyuho
Copy link
Contributor Author

gyuho commented Feb 11, 2016

Thanks. Will merge in green lights.

@@ -33,6 +34,9 @@ type Cluster interface {
// List lists the current cluster membership.
MemberList(ctx context.Context) (*MemberListResponse, error)

// Leader returns the current leader member.
Copy link
Contributor

Choose a reason for hiding this comment

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

godoc?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I missed this comment. You mean to make the comment MemberLeader? I can change that in a separate PR with the fix for MemberList as well.

gyuho added a commit that referenced this pull request Feb 11, 2016
etcdserver: include IsLeader in etcdserverpb.Member
@gyuho gyuho merged commit a8bfd12 into etcd-io:master Feb 11, 2016
@gyuho gyuho deleted the leader_api branch February 11, 2016 21:39
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.

3 participants