From 00a4da4badd5f8262ad29c2d41dabda7e4317e1b Mon Sep 17 00:00:00 2001 From: David Finkel Date: Thu, 24 Mar 2022 18:23:18 -0400 Subject: [PATCH 1/3] campaign: rename error variable to aqErr The error for the switch covering most of the body of the main loop of Acquire shouldn't be called `err`. Give it a better name. --- campaign.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/campaign.go b/campaign.go index 4bfd4fe..5ca7c9e 100644 --- a/campaign.go +++ b/campaign.go @@ -58,7 +58,7 @@ func (c Config) Acquire(ctx context.Context) error { wg := sync.WaitGroup{} defer wg.Wait() for { - switch acquiredEntry, err := cmp.acquireOnce(ctx, lastEntry); err { + switch acquiredEntry, aqErr := cmp.acquireOnce(ctx, lastEntry); aqErr { case nil: b.Reset() if c.LeaderChanged != nil { @@ -108,7 +108,7 @@ func (c Config) Acquire(ctx context.Context) error { // WriteEntry should fail immediately. } default: - return err + return aqErr } if lastEntry.ElectionNumber == entry.NoElections { // There haven't been any elections yet, just loop and From 5b94267d57e36889ddf0eb8986a78feb579eaefa Mon Sep 17 00:00:00 2001 From: David Finkel Date: Thu, 24 Mar 2022 18:25:00 -0400 Subject: [PATCH 2/3] gcs: use the + for concatenattion in Error() Don't use fmt.Sprintf when you're just concatenating two strings. --- gcs/decider.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gcs/decider.go b/gcs/decider.go index ce0997e..b07c482 100644 --- a/gcs/decider.go +++ b/gcs/decider.go @@ -150,7 +150,7 @@ type failedAcquisitionErr struct { } func (f *failedAcquisitionErr) Error() string { - return fmt.Sprintf("failed to acquire lock: %s", f.err) + return "failed to acquire lock: " + f.err.Error() } func (f *failedAcquisitionErr) Unwrap() error { From dbf3acb181705be0bd0beed34fb5ce50bf343057 Mon Sep 17 00:00:00 2001 From: David Finkel Date: Thu, 24 Mar 2022 17:59:48 -0400 Subject: [PATCH 3/3] Readme: reword a paragraph so it's not misleading The description of OnElected, OnOusting, and LeaderChanged are a bit misleading because of an interstitial clause. Rewrite that paragraph and break it up into two. --- README.md | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index 7cb9bec..f020a67 100644 --- a/README.md +++ b/README.md @@ -20,10 +20,17 @@ Currently, this implementation only has two implementations: The [`Config`] struct contains callbacks and tunables for the leader election "campaign". -Each process that would like to acquire leadership must register callbacks for -all three of `OnElected`, `OnOusting` and `LeaderChanged`, as well as specify -unique `LeaderID` and `HostPort`s (the latter two are used for communication, so -some use-cases may be able to ignore them) +Each "candidate" contending for leadership must register an `OnElected` callback +and a `LeaderID` (which is often a random string). Additionally, it is +recommended to specify `HostPort`, which makes it possible to leverage the +`legrpc` package and have other clients using the `WatchConfig{}.Watch()` method +connect to the current leader. (it can also be useful for debugging) + +Optionally, one can specify `LeaderChanged` and `OnOusting` callbacks which are +called when the current leader changes and an instance has lost/ceded its +election, respectively. If set, `OnOusting` is guaranteed to be called after +losing leadership, whether that's by losing the election, or by its context +being canceled. The `TermLength` is the length of time that a leader acquires leadership for, and the length of any extension. This duration should be long enough to get