quorum: port single group joint commit #394

jayzhan211 · 2020-08-26T01:48:31Z

Signed-off-by: accelsao bebe8277@gmail.com

Signed-off-by: accelsao <bebe8277@gmail.com>

BusyJay · 2020-08-26T05:50:22Z

src/quorum/testdata/joint_commit.txt

+committed cfg=(1,2,3) cfgj=zero idx=(100,101,99)
+----
+       idx
+x>       [0]100 (id=1)


Is it group id? How about hiding 0 by default? So that the file is the same as etcd.

I set the width larger for the space of group id

Can they be removed?

Can they be removed?

Which one? space, group id or this describe?

Space. Better make the file identical with etcd's.

Fullstop000 · 2020-08-26T06:20:33Z

src/quorum/majority.rs

+    /// Describe returns a (multi-line) representation of the commit indexes for the
+    /// given lookuper.
+    pub fn describe(&self, l: &impl AckedIndexer) -> String {


Prefer to add a sample output

Fullstop000 · 2020-08-26T06:34:03Z

src/quorum/datadriven_test.rs

+use datadriven::{run_test, TestData};
+
+#[test]
+fn test_data_driven_quorum() -> anyhow::Result<()> {


Is the returning value necessary?

Yes, we return lots of Result in run_test, such as read_to_string, OpenOptions::new()

Signed-off-by: accelsao <bebe8277@gmail.com>

NingLin-P · 2020-08-27T04:54:34Z

src/quorum/majority.rs

+    /// >            99 (id=3)
+    /// 100
+    /// ```
+    pub fn describe(&self, l: &impl AckedIndexer) -> String {


Could you add an unit test for it?

Signed-off-by: accelsao <bebe8277@gmail.com>

NingLin-P

LGTM, thanks!

Fullstop000

LGTM

jayzhan211 · 2020-08-31T06:26:07Z

@BusyJay PTAL, thanks

Signed-off-by: accelsao <bebe8277@gmail.com>

jayzhan211 · 2020-08-31T13:52:17Z

src/quorum/majority.rs

+            r#"       idx
+?        0    (id=1)
+x>     120    (id=2)
+>    [1]120    (id=3)


Because the width in etcd is 5, so this cant be aligned if the width of id is over 5 (including group id information)

jayzhan211 · 2020-09-06T23:56:50Z

@BusyJay PTAL, thanks

BusyJay · 2020-09-14T12:17:33Z

src/quorum/joint.rs

@@ -23,6 +23,11 @@ impl Configuration {
        }
    }

+    /// Creates a new configuration using the given MajorityConfigs.
+    pub fn new_joint_from_majorities(incoming: MajorityConfig, outgoing: MajorityConfig) -> Self {


Should be pub(crate).

BusyJay · 2020-09-14T12:19:21Z

src/quorum/majority.rs

+    /// >            99 (id=3)
+    /// 100
+    /// ```
+    pub fn describe(&self, l: &impl AckedIndexer) -> String {


Should be pub(crate).

BusyJay · 2020-09-14T12:21:29Z

src/quorum/majority.rs

+        // plot this as sort of a progress bar). The actual code is a bit more
+        // complicated and also makes sure that equal index => equal bar.
+
+        let mut info = vec![];


You can use Vec::with_capacity(n).

BusyJay · 2020-09-14T12:22:31Z

src/quorum/majority.rs

+
+        let mut info = vec![];
+
+        for id in self.raw_slice() {


Why not for id in &self.voters?

BusyJay · 2020-09-14T12:24:06Z

src/quorum/majority.rs

+            info.push(Tup { id, idx, bar: 0 })
+        }
+
+        info.sort_by(|a, b| match a.idx.cmp(&b.idx) {


You mean (a.idx, a.id).cmp(&(b.idx, b.id))?

BusyJay · 2020-09-14T12:30:46Z

src/quorum/majority.rs

+        for tup in info {
+            let string;
+            if let Some(idx) = tup.idx {
+                string = "x".repeat(tup.bar)


How about using buf directly instead of creating a temporary string?

BusyJay · 2020-09-14T12:35:22Z

src/quorum/majority.rs

+                string = "x".repeat(tup.bar)
+                    + ">"
+                    + " ".repeat(n - tup.bar).as_str()
+                    + format!(" {:>5}    (id={})\n", format!("{}", idx), tup.id).as_str();


Redundant code, can be extracted as write!(buf, " {:>5} (id={})\n", tup.idx.unwrap_or_default(), tup.id).unwrap().

I still use branch to determine the prefix of the string

I fail to get width with
writeln!(buf, " {:>5} (id={})", idx, tup.id) .expect("Error occurred while trying to write in String"); and
writeln!(buf, " {:>5} (id={})", Index::default(), tup.id) .expect("Error occurred while trying to write in String");

Can you talk more about the error you meet? Or you can show me a picture in slack.

the following is the result with this code writeln!(buf, " {:>5} (id={})", idx, tup.id),
the green one is the expected output.

with this code writeln!(buf, " {:>5} (id={})", format!("{}", idx), tup.id), I can get the green output

It seems the problem may be in Index::Display
https://stackoverflow.com/questions/56826464/why-is-the-width-ignored-for-my-custom-formatter-implementation.
And I prefer using format!() instead rewriting Display :D

Signed-off-by: accelsao <jayzhan211@gmail.com>

BusyJay · 2020-09-15T06:46:03Z

src/quorum/joint.rs

+
+    /// Describe returns a (multi-line) representation of the commit indexes for the
+    /// given lookuper.
+    pub fn describe(&self, l: &impl AckedIndexer) -> String {


Should be pub(crate).

Any updates?

BusyJay · 2020-09-15T06:52:35Z

src/quorum/majority.rs

+            match tup.idx {
+                Some(idx) => write!(
+                    buf,
+                    "x".repeat(tup.bar)


Don't mix write! and +. Just use write! is easy to read.

BusyJay · 2020-09-15T06:55:45Z

src/quorum.rs

 pub struct Index {
    pub index: u64,
    pub group_id: u64,
 }

+impl PartialEq for Index {


It doesn't feel right to implement PartialEq and its family traits for the Index. As the same index with different group id should not be treat as the same Index. The comparison rule is limited to context and should not apply universally.

Signed-off-by: accelsao <jayzhan211@gmail.com>

BusyJay · 2020-09-15T08:45:32Z

src/quorum/joint.rs

@@ -23,6 +23,15 @@ impl Configuration {
        }
    }

+    // use in test only
+    #[allow(dead_code)]


You can use #[cfg(test)] instead.

BusyJay · 2020-09-15T09:22:20Z

src/quorum/datadriven_test.rs

+        // be empty) and the second one is used iff joint is true.
+        let mut joint = false;
+
+        // The committed indexes for the nodes in the config in the order in


I think the comment is for idxs.

BusyJay · 2020-09-15T09:30:38Z

src/quorum/datadriven_test.rs

+            let mut l = AckIndexer::default();
+            // next to consume from idxs
+            let mut p: usize = 0;
+            let mut ids = ids.to_vec();


How about for id in ids.iter().chain(idsj)?

BusyJay · 2020-09-15T09:33:31Z

src/quorum/datadriven_test.rs

+        };
+
+        let input = idxs.clone();
+        let voters: HashSet<u64> = JointConfig::new_joint_from_majorities(c.clone(), cj.clone())


.ids().len() should be enough.

BusyJay · 2020-09-15T09:35:36Z

src/quorum/datadriven_test.rs

+#[test]
+fn test_data_driven_quorum() -> anyhow::Result<()> {
+    let logger = default_logger();
+    fn test_quorum(data: &TestData) -> String {


Better move it out of function for readability.

Signed-off-by: accelsao <jayzhan211@gmail.com>

BusyJay

Rest LGTM

BusyJay · 2020-09-16T09:18:21Z

src/quorum/joint.rs

+
+    /// Describe returns a (multi-line) representation of the commit indexes for the
+    /// given lookuper.
+    pub fn describe(&self, l: &impl AckedIndexer) -> String {


Any updates?

BusyJay · 2020-09-16T09:23:26Z

src/quorum/majority.rs

+    /// >            99 (id=3)
+    /// 100
+    /// ```
+    pub(crate) fn describe(&self, l: &impl AckedIndexer) -> String {


Perhaps we can mark it with #[cfg(test)] so that we don't need another test case to test it.

Signed-off-by: accelsao <jayzhan211@gmail.com>

quorum: add joint_commit

00c7154

Signed-off-by: accelsao <bebe8277@gmail.com>

BusyJay reviewed Aug 26, 2020

View reviewed changes

BusyJay requested a review from NingLin-P August 26, 2020 05:50

Fullstop000 reviewed Aug 26, 2020

View reviewed changes

quorum: address comment

0938359

Signed-off-by: accelsao <bebe8277@gmail.com>

NingLin-P reviewed Aug 27, 2020

View reviewed changes

310552025atNYCU added 2 commits August 27, 2020 14:24

quorum: add unit test for describe

73c3846

Signed-off-by: accelsao <bebe8277@gmail.com>

quorum: remove comment

0d6cf7a

Signed-off-by: accelsao <bebe8277@gmail.com>

jayzhan211 requested review from BusyJay, Fullstop000 and NingLin-P August 27, 2020 10:07

NingLin-P previously approved these changes Aug 28, 2020

View reviewed changes

Fullstop000 previously approved these changes Aug 28, 2020

View reviewed changes

310552025atNYCU added 2 commits August 31, 2020 19:53

quorum: add joint commit test data

a2a8936

Signed-off-by: accelsao <bebe8277@gmail.com>

quorum: test describe in raw string

fde8654

Signed-off-by: accelsao <bebe8277@gmail.com>

jayzhan211 dismissed stale reviews from Fullstop000 and NingLin-P via fde8654 August 31, 2020 12:38

jayzhan211 requested review from NingLin-P and Fullstop000 August 31, 2020 13:09

jayzhan211 commented Aug 31, 2020

View reviewed changes

BusyJay reviewed Sep 14, 2020

View reviewed changes

address comment

4158b56

Signed-off-by: accelsao <jayzhan211@gmail.com>

BusyJay reviewed Sep 15, 2020

View reviewed changes

address comment

2d41b43

Signed-off-by: accelsao <jayzhan211@gmail.com>

BusyJay reviewed Sep 15, 2020

View reviewed changes

310552025atNYCU added 2 commits September 15, 2020 19:39

address comment

516a09d

Signed-off-by: accelsao <jayzhan211@gmail.com>

address clippy

cec30ad

Signed-off-by: accelsao <jayzhan211@gmail.com>

BusyJay reviewed Sep 16, 2020

View reviewed changes

address comment

0c846a1

Signed-off-by: accelsao <jayzhan211@gmail.com>

BusyJay approved these changes Sep 16, 2020

View reviewed changes

NingLin-P approved these changes Sep 17, 2020

View reviewed changes

BusyJay merged commit 0c53e7b into tikv:master Sep 17, 2020

jayzhan211 deleted the single-commit branch September 17, 2020 08:12

quorum: port single group joint commit #394

quorum: port single group joint commit #394

Conversation

jayzhan211 commented Aug 26, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NingLin-P left a comment

Choose a reason for hiding this comment

Fullstop000 left a comment

Choose a reason for hiding this comment

jayzhan211 commented Aug 31, 2020

jayzhan211 Aug 31, 2020 • edited Loading

Choose a reason for hiding this comment

jayzhan211 commented Sep 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jayzhan211 Sep 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BusyJay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jayzhan211 Aug 31, 2020 •

edited

Loading

jayzhan211 Sep 15, 2020 •

edited

Loading