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

stepWait not used in ProposeConfChange #43

Open
lavacat opened this issue Apr 11, 2023 · 1 comment · May be fixed by #44
Open

stepWait not used in ProposeConfChange #43

lavacat opened this issue Apr 11, 2023 · 1 comment · May be fixed by #44

Comments

@lavacat
Copy link

lavacat commented Apr 11, 2023

stepWait was implemented to fail-fast in Propose method.
But it was never used in ProposeConfChange method.

Question was raised about it on original PR.
Also, it's was causing timeout in ExampleCluster_memberAddAsLearner test. comment

This isn't a critical issue, because in production ProposeConfChange isn't usually called consecutively. But it seams to me that stepWait was missed for ProposeConfChange method.

lavacat pushed a commit to lavacat/raft that referenced this issue Apr 11, 2023
fixes: etcd-io#43
Signed-off-by: Bogdan Kanivets <bkanivets@apple.com>
@folays
Copy link

folays commented Apr 18, 2024

OP wonders about ProposeConfchange().

Besides ProposeConfChange(), Propose() has also some problems which probably stems from the same origins.

I will talk only about cluster where DisableProposalForwarding = false, because = true has it own set of unfollow-ability of failed proposals. It helps drafting things quickly, but it's bad in ensuring reliability.

(*node).Propose() method is widly undocumented on what can it be expected of it.
An API is like a contract, and without specifications on it, what are its limitation etc..., it's like there is no guarantee at all to have it to eventually do something in any specific timeframe, so it's unuable in any serious production environment without first reading all code path it branches into to figure out those.

.Propose() only specification is Note that proposals can be lost without notice, therefore it is user's job to ensure proposal retries. which does not help at all. No words on when/how the loss can be noticed and/or assumed.

It appears that the set of predicates could hold for .Propose() :

  • when it has been called on a current leader (rd.SoftState.Lead == r.id)
  • and .Propose() returned err == nil
    • that means that it is guaranteed to be proposed and ultimately commited/applied...
  • ...guarantee which would hold as long as "this" leader consider itself to have leadership
    • otherwise you should trigger cancellation/timeout of the ongoing proposal waiting status
  • which would mean we seem to be able to assume that the Proposal does not need retrial in the meantime if the rest of the conditions hold

.ProposeConfChange() is worse, and its API behavior for callers seems to be "hope for the best", "retry after some unspecified time if commit does not appears in .CommittedEntries[]"

The need is somewhat simple : people wants to have Propose() and ProposeConfChange() succeed as soon as possible, which entrails to have them fail "fast" is they are failing, which could be better rephrased as : having a mechanism (or documentation) on how to known for a given Proposal, As-Soon-As-Possible, that it failed/dropped, so that the caller can (at its discrete choice) either quickly return the error, or quickly retry.

I understand that the protocol and logic behing it implies that there is no timeframe guarantee on the commitability or failure of Proposals.

It's OKAY for something to be in progress, and having to wait for an indeterminate amount of time for it to progress, as long as further progress is expected.

What's not okay if for something to definitely having stopped progression, where failure is certain (absence of further progress is certain), but no mechanism is being provided to detect/notify the certainty of the failure.

What indeed feels unatural is, when a "proposal requester" (e.g. a GRPC caller whishing to have some data commited to the Raft logs) DIRECTLY requested the leader, and when this leader reached a point of knowledge of the wherabouts of the ongoing Proposal, specifically reached knowledge of its failure, there is zero documentation on how to detect that.

Note : I'm saying "a / the leader", because if the leader is splitbrained from the majority and doesn't know it yet, it's okay for it to hold "proposal requester" in holding status, because it would still consider itself as "a leader" whilst being only really an "incapacitated leader", whereas an "operative leader with majority" has been elected on the other splitbrain side, but of course the incapacitated wouldn't know it before any timeout occurs.

I think that OP's ProposeConfChange is even worse that Propose(), because ProposeConfChange() deep calls .stepWaitWithOption() with wait = false, which means that if the leader ITSELF decides to drop the proposal, the caller of ProposeConfChange() would have no way of knowing it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants