Remove Progress.is_learner #119

Hoverbear · 2018-09-07T22:37:13Z

Remove the is_learner value from Progress.

Rationale

In the process of Joint Consensus (#101) we will have two different Configuration sets during some points in operation.

In #108 we made Progress a single set (instead of having separate sets for voters and learners). Progress still held an is_learner value, which meant we had two ways to determine if a node was a learner (via the ProgressSet.has_learner() and Progress.is_learner). This is not necessary.

We can use ProgressSet.has_learner(id) and ProgressSet.has_voter(id) exclusively.

Benefits

Doing this helps minimize complexity, and reduces places we keep track of voter status.

Future Work

In the future we may be in a joint consensus state, meaning that the Progress.is_learner value could be incorrect (If you promoted a learner, then the old configuration still has it has a non-voter).

This internalizes the concept of the is_learner() so we can minimize possible bug surface.

overvenus · 2018-09-26T09:24:20Z

benches/suites/progress_set.rs

+        b.iter(|| ProgressSet::new());
+    };
+
+    c.bench_function(&format!("ProgressSet::new"), bench);


How about c.bench_function("ProgressSet::new", bench)?

Hoverbear · 2018-09-27T18:13:15Z

src/raft.rs

@@ -1993,12 +1993,13 @@ impl<T: Storage> Raft<T> {
    fn check_quorum_active(&mut self) -> bool {
        let self_id = self.id;
        let mut act = 0;
+        let learners = self.prs().learner_ids().clone();


The clone here seems the best way, as much as I hate it. Since we use .mut_prs() and iterate we have a mutable borrow out for self.

We copy this HashSet<u64> which should only have learner _ids() in it, which should be not unreasonably large. An alternative is we can clone voter_ids() which should always be reasonable even if there are many learners. The downside being we always have the cost of the voters (3-7 usually).

You can use take_prs to get around the lifetime check just as other places already did.

I always feel so dirty doing this. 🤣 But yes you're right.

hicqu · 2018-09-29T07:23:39Z

src/raft.rs

-        if let Some(progress) = self.prs().get(id) {
-            // If progress.is_learner == learner, then it's already inserted as what it should be, return early to avoid error.
-            if progress.is_learner == learner {
+        if self.prs().get(id).is_some() {


It's not necessary any more.

Agree. insert_learner and similar functions do the check already. We can check the result to reflect errors.

Ok. I will make this function (which is private) return a Result<()>. I would like to make the callers return a Result<()> in 0.5.0.

See #129.

hicqu · 2018-09-29T07:24:41Z

rest LGTM. Nice work!

Hoverbear · 2018-10-10T00:43:51Z

@BusyJay Can you take another look?

hicqu · 2018-10-11T09:48:36Z

PTAL @BusyJay .

src/progress.rs

Hoverbear · 2018-10-17T15:11:59Z

@BusyJay Please take a look again, the test name is fixed.

tests/integration_cases/test_raft.rs

src/progress.rs

siddontang

Rest LGTM

Hoverbear · 2018-10-20T14:12:39Z

@siddontang Resolved, PTAL. I think it's ready for merge.

121: Move poll & quorum functionality into ProgressSet r=Hoverbear a=Hoverbear Based off #119. **You are seeing that change set as well here, so go review that, then this after merging it! :)** Migrate poll and quorum related functionality inside `ProgressSet`. These functions operated on the `ProgressSet` anyways, they were just scoped to the `Raft`. # Rationale With the introduction of Joint Consensus, the idea of a Quorum is no longer always a single value. (If the cluster is in a joint state it actually has two quorums. The old configuration and the new one.) In the interest of isolating responsibility and dulling sharp edges, this PR migrates code the following functionality: * `check_quorum_active`: Migrated, it was doing a mutable fold on the peers and resetting their recent active, then returning if there was enough for quorum. The `ProgressSet` can do this itself, and it means we can avoid having to return some enum of either `(quorum)` or `(new_quorum, old_quorum)`. The `Raft` module need not care. * `poll`: Was split into a mutating vote-setter function, and a check function (non mutating). The check function returns a semantically meaningful enum now. This was a good idea since `poll(vote) >= quorum`, while still valid, may be comparing tuples or values. Internalizing this logic and instead returning a semantic value helps with understanding. * `minimum_committed_index`: Since in a joint consensus state we need to check the minimum committed index of both old and new, we can internalize this check and just return a bool. # Benefits This PR essentially amounts to API cleaning. It doesn't change any behavior, but it's a change we should do to make future work easier. # Future Work When Joint Consensus (#101 ) lands it will change these new functions to be joint-aware and correctly return values regardless of the configuration state. Co-authored-by: Hoverbear <andrew@hoverbear.org> Co-authored-by: Hoverbear <operator@hoverbear.org> Co-authored-by: A. Hobden <andrew@hoverbear.org> Co-authored-by: A. Hobden <operator@hoverbear.org> Co-authored-by: qupeng <onlyqupeng@gmail.com>

Hoverbear added Enhancement An improvement to existing code. learner Anything related to learner mechanism. labels Sep 7, 2018

Hoverbear added this to the 0.4.0 milestone Sep 7, 2018

Hoverbear self-assigned this Sep 7, 2018

Hoverbear mentioned this pull request Sep 7, 2018

Use a single set of Progresses for ProgressSet. #108

Merged

Hoverbear added the Feature Related to a major feature. label Sep 7, 2018

Hoverbear modified the milestones: 0.4.0, 0.5.0 Sep 11, 2018

Hoverbear force-pushed the remove_is_learner branch 3 times, most recently from 2810600 to 4dd2e63 Compare September 12, 2018 00:03

Hoverbear mentioned this pull request Sep 12, 2018

Move poll & quorum functionality into ProgressSet #121

Merged

Hoverbear force-pushed the remove_is_learner branch 5 times, most recently from d861453 to f8f30f5 Compare September 19, 2018 00:11

Hoverbear force-pushed the remove_is_learner branch 2 times, most recently from c468ee8 to a744827 Compare September 25, 2018 17:35

overvenus reviewed Sep 26, 2018

View reviewed changes

Hoverbear force-pushed the remove_is_learner branch 2 times, most recently from e250876 to 546b926 Compare September 26, 2018 17:17

Remove Progress.is_learner and depend on ProgressSet Configuration.

911a27a

Hoverbear force-pushed the remove_is_learner branch from 546b926 to 911a27a Compare September 27, 2018 16:59

Hoverbear commented Sep 27, 2018

View reviewed changes

Refine promote_learner

2fa4a65

hicqu reviewed Sep 29, 2018

View reviewed changes

fmt

0580ffa

Hoverbear dismissed hicqu’s stale review via 0580ffa October 10, 2018 00:43

hicqu previously approved these changes Oct 10, 2018

View reviewed changes

BusyJay reviewed Oct 13, 2018

View reviewed changes

src/progress.rs Show resolved Hide resolved

Fix test

623ed68

Hoverbear dismissed hicqu’s stale review via 623ed68 October 17, 2018 15:11

Add raft state mutation test

22e6d02

Hoverbear force-pushed the remove_is_learner branch from 3e3d1b3 to 22e6d02 Compare October 18, 2018 15:05

Hoverbear commented Oct 18, 2018

View reviewed changes

tests/integration_cases/test_raft.rs Show resolved Hide resolved

BusyJay previously approved these changes Oct 19, 2018

View reviewed changes

siddontang reviewed Oct 20, 2018

View reviewed changes

src/progress.rs Outdated Show resolved Hide resolved

siddontang previously approved these changes Oct 20, 2018

View reviewed changes

Fix error type

68f90e6

Hoverbear dismissed stale reviews from siddontang and BusyJay via 68f90e6 October 20, 2018 14:11

siddontang approved these changes Oct 22, 2018

View reviewed changes

Hoverbear merged commit dcba27f into master Oct 22, 2018

Hoverbear deleted the remove_is_learner branch October 22, 2018 14:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove Progress.is_learner #119

Remove Progress.is_learner #119

Hoverbear commented Sep 7, 2018 •

edited

Loading

overvenus Sep 26, 2018

Hoverbear Sep 27, 2018 •

edited

Loading

BusyJay Oct 1, 2018

Hoverbear Oct 1, 2018

hicqu Sep 29, 2018

BusyJay Oct 1, 2018

Hoverbear Oct 1, 2018 •

edited

Loading

hicqu commented Sep 29, 2018

Hoverbear commented Oct 10, 2018

hicqu commented Oct 11, 2018

Hoverbear commented Oct 17, 2018

siddontang left a comment

Hoverbear commented Oct 20, 2018

Remove Progress.is_learner #119

Remove Progress.is_learner #119

Conversation

Hoverbear commented Sep 7, 2018 • edited Loading

Rationale

Benefits

Future Work

overvenus Sep 26, 2018

Choose a reason for hiding this comment

Hoverbear Sep 27, 2018 • edited Loading

Choose a reason for hiding this comment

BusyJay Oct 1, 2018

Choose a reason for hiding this comment

Hoverbear Oct 1, 2018

Choose a reason for hiding this comment

hicqu Sep 29, 2018

Choose a reason for hiding this comment

BusyJay Oct 1, 2018

Choose a reason for hiding this comment

Hoverbear Oct 1, 2018 • edited Loading

Choose a reason for hiding this comment

hicqu commented Sep 29, 2018

Hoverbear commented Oct 10, 2018

hicqu commented Oct 11, 2018

Hoverbear commented Oct 17, 2018

siddontang left a comment

Choose a reason for hiding this comment

Hoverbear commented Oct 20, 2018

Hoverbear commented Sep 7, 2018 •

edited

Loading

Hoverbear Sep 27, 2018 •

edited

Loading

Hoverbear Oct 1, 2018 •

edited

Loading