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

Election RPC service #7634

Merged
merged 11 commits into from
Apr 8, 2017
Merged

Conversation

heyitsanthony
Copy link
Contributor

Has some updates to the Election API to make it more usable with transactions / similar to distributed Mutexes. Also includes a fix for watches with v3client.

// Proclaim updates the leader's posted value with a new value.
rpc Proclaim(ProclaimRequest) returns (ProclaimResponse) {
option (google.api.http) = {
post: "/v3alpha/election/campaigujkjn"
Copy link
Contributor

Choose a reason for hiding this comment

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

/election/ProcaimRequest

}

// NewElection returns a new election on a given key prefix.
func NewElection(s *Session, pfx string) *Election {
return &Election{session: s, keyPrefix: pfx + "/"}
}

// ResumeElection intiializes an election with a known leader.
Copy link
Member

Choose a reason for hiding this comment

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

s/intiializes/initializes/

cmp := v3.Compare(v3.CreateRevision(e.leaderKey), "=", e.leaderRev)
resp, err := client.Txn(ctx).If(cmp).Then(v3.OpDelete(e.leaderKey)).Commit()
if err != nil {
e.hdr = resp.Header
Copy link
Member

Choose a reason for hiding this comment

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

I think the Txn returns nil resp if err is not nil; Accessing resp.Header will trigger a panic.

embed/serve.go Outdated
@@ -71,6 +73,7 @@ func (sctx *serveCtx) serve(s *etcdserver.EtcdServer, tlscfg *tls.Config, handle

if sctx.insecure {
gs := v3rpc.Server(s, nil)
v3electionpb.RegisterElectionServer(gs, v3election.NewElectionServer(v3client.New(s)))
Copy link
Member

Choose a reason for hiding this comment

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

I wonder why can't NewElectionServer and NewLockServer share the same v3client?

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 doesn't really make a difference since it's all pass-through, I wanted to save a line of code

Copy link
Member

Choose a reason for hiding this comment

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

sounds good.

@heyitsanthony
Copy link
Contributor Author

All fixed up. CI failures from #7609 and #7652

if lerr != nil {
return nil, lerr
}
return &epb.LeaderResponse{Header: l.Header, Kv: l.Kvs[0]}, nil
Copy link
Member

Choose a reason for hiding this comment

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

Are we expecting the client that this api to parse leader name out of kv?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. It's returning the full kv so it's possible to do txns on the leader key and revision. The client shouldn't care about what's in the key name in particular, just that it has some way to refer to the key by name.

Copy link
Member

Choose a reason for hiding this comment

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

is it the user responsibility to understand what kv contains and how to use it? if yes, would it be nicer to have response that that's more descriptive something like following?

message LeaderResponse {
   etcdserverpb.ResponseHeader header = 1;
   // comment
   bytes key = 2;
   // comment
   int64 rev = 3;
   // comment
   bytes value = 4;
} 

Just a thought.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The assumption is the caller for Leader expects to get a key that represents the latest leader post. There's already a data type for keys and it's mvccpb.KeyValue and the client should already know what a key is.

The metadata is still useful since creation revision, modification revision, and version can all be used to reason about the status of the leader. I don't understand what the advantage is to having a totally separate data type for this-- it's a new data type that can't be used like an ordinary key and it lacks key metadata.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe separate data structure is not useful. However, my main concern is that would the caller/user know exactly what to do with the kv data type and its metadata?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, can add some examples for the Lock and Election RPCs in another PR.

option (gogoproto.marshaler_all) = true;
option (gogoproto.unmarshaler_all) = true;

// The election service exposes client-side election facilities as a gRPC interface.
Copy link
Contributor

Choose a reason for hiding this comment

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

remove the mentioning of client-side? i do not think the user should care about this.

resp, err := client.Txn(ctx).If(cmp).Then(v3.OpDelete(e.leaderKey)).Commit()
if err == nil {
e.hdr = resp.Header
}
e.leaderKey = ""
e.leaderSession = nil
return err
}

// Leader returns the leader value for the current election.
Copy link
Member

Choose a reason for hiding this comment

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

update the comment to reflect the change? I don't think leader just return leader value anymore.

@heyitsanthony
Copy link
Contributor Author

ok all fixed

@fanminshi
Copy link
Member

lgtm

@heyitsanthony
Copy link
Contributor Author

failing proxy-ci on grpc/grpc-go#1167

@xiang90
Copy link
Contributor

xiang90 commented Apr 4, 2017

lgtm

Anthony Romano added 11 commits April 7, 2017 16:36
The full information about the leader's key is necessary to
safely use elections with transactions. Instead of returning
only the value on Leader(), return the entire GetResposne.
…atches

Addresses a case where two clients share the same lease. A client resigns but
disconnects / crashes and doesn't realize it. Another client reuses the
lease and gets leadership with a new key. The old client comes back and
tries to resign again, revoking the new leadership of the new client.
If a client already knows it holds leadership, let it create an
election object with its leadership information.
The Get for the leader key will fetch based on the latest revision
instead of the deletion revision, missing leader updates between
the delete and the Get.

Although it's usually safe to skip these updates since they're
stale, it makes testing more difficult and in some cases the
full leader update history is desirable.
gRPC will replace empty strings with nil, but for the embedded case it's
possible for []byte{} to slip in and confuse the single key / >= key
watch logic.
@heyitsanthony heyitsanthony merged commit 25acdbf into etcd-io:master Apr 8, 2017
@heyitsanthony heyitsanthony deleted the election-rpc branch April 8, 2017 03:03
@gyuho gyuho changed the title Election RPC service *: Election RPC service Apr 25, 2017
@gyuho gyuho changed the title *: Election RPC service Election RPC service Apr 25, 2017
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.

4 participants