-
Notifications
You must be signed in to change notification settings - Fork 37
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
fix(paxos): Proposers don't re-commit entries before the latest checkpoint #1615
fix(paxos): Proposers don't re-commit entries before the latest checkpoint #1615
Conversation
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.
Looks like there are some conflicts too.
This is just an optimization right? The acceptors would just ignore the re-commits lower than the checkpoint? |
Don't think so, acceptors have no logic to ignore things based on the checkpoint, just to remove things from the log. |
|
hydro_test/src/cluster/paxos.rs
Outdated
@@ -483,7 +483,22 @@ fn recommit_after_leader_election<'a, P: PaxosPayload>( | |||
Stream<P2a<P>, Tick<Cluster<'a, Proposer>>, Bounded, NoOrder>, | |||
Optional<usize, Tick<Cluster<'a, Proposer>>, Bounded>, | |||
) { | |||
let p_p1b_max_checkpoint = accepted_logs.clone().fold_commutative( |
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.
In theory you could replace this with a .map().flatten().max()
.
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.
Doesn't this change behavior from a stream that contains either None or Some(checkpoint) to an Optional stream that may contain checkpoint? Because then the later join with p_log_to_try_commit
won't go through if there is no checkpoint right? Without the flatten maybe it's ok?
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.
True! You can fix this by adding an addition .into_singleton()
which should convert the Optional<T>
into Singleton<Option<T>>
.
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.
I'll just do map().max()
, since Some(anything) > None so it works out
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.
One potential improvement, feel free to just merge though.
U right |
Paxos proposers assign no-ops to holes in the log after receiving a quorum of p1bs. Acceptors delete log entries before the latest checkpoint, which means they will show up as holes in the proposer. The proposer should not propose any operations before the most recent checkpoint in a quorum of p1bs.