implement CommittedEntries pagination #161

Fullstop000 · 2018-12-26T12:45:11Z

Fixes #105

Fullstop000 · 2018-12-26T13:00:12Z

I got several warnings and errors complaining about some deprecated lint names when executing cargo clippy --all -- -D clippy in rustc 1.33.0-nightly (14b96659e 2018-12-25).
warnings like :

warning: lint name `clippy` is deprecated and does not have an effect anymore. Use: clippy::all
  |
  = note: requested on the command line with `-D clippy`

warning: lint name `clippy` is deprecated and may not have an effect in the future. Also `cfg_attr(cargo-clippy)` won't be necessary anymore
 --> src/eraftpb.rs:6:45
  |
6 | #![cfg_attr(feature = "cargo-clippy", allow(clippy))]
  |                                             ^^^^^^ help: change it to: `clippy::all`
  |
  = note: #[warn(renamed_and_removed_lints)] on by default

erros like below :

error: methods called `new` usually return `Self`
   --> src/raft.rs:208:5
    |
208 | /     pub fn new(c: &Config, store: T) -> Result<Raft<T>> {
209 | |         c.validate()?;
210 | |         let rs = store.initial_state()?;
211 | |         let conf_state = &rs.conf_state;
...   |
292 | |         Ok(r)
293 | |     }
    | |_____^
    |
    = note: `-D clippy::new-ret-no-self` implied by `-D clippy`
    = help: for further information visit https://rust-lang-nursery.github.io/rust-clippy/master/index.html#new_ret_no_self

Probably I need some guidance about this since I'm a rust cookie ( just wrote this PR as my first shot in rust )

Hoverbear · 2018-12-31T22:21:59Z

Hi @Fullstop000 I'll deal with those errors. :) Sorry about that!

src/raft_log.rs

Hoverbear

Hi! Thanks so much for tacking this! We really appreciate it! 🎉

In general it LGTM, but I notice in the etcd PR (here there is a second test added. Can you explain the choice to not add this one?

Fullstop000 · 2019-01-02T03:50:11Z

@Hoverbear the second test TestAppendPagination is to test whether MaxSizePerMsg is able to cap the total byte size of entries in each message.
Do I need to add this as I don't clearly know whether we've tested this or not ( seems not ) ?

And I find that etcd-raft has added a new config parameter MaxCommittedSizePerReady to control this instead of MsgSizePerMsg.
etcd-io/etcd@e4af2be

tests/integration_cases/test_raw_node.rs

Hoverbear · 2019-01-04T19:01:14Z

@Fullstop000 Great catch. I think this is a more clear functionality. @hicqu can you take a look?

Fullstop000 · 2019-01-05T04:16:41Z

I'll use the new parameter max_committed_size_per_ready instead. And the test for max_size_per_msg will be added.

Hoverbear

Rest LGTM.

siddontang · 2019-01-30T04:11:21Z

Ping @BusyJay @hicqu

src/config.rs

Hoverbear

Generally LGTM, only a small lint related comment.

src/raw_node.rs

BusyJay · 2019-02-13T02:49:08Z

src/raft_log.rs

+            new_entry_with_data(5, 1, data.clone()),
+            new_entry_with_data(6, 1, data.clone()),
+        ];
+        let unit_msg_size = u64::from(protobuf::Message::compute_size(&new_entry_with_data(


What about &ents[0]?

BusyJay · 2019-02-13T03:12:22Z

src/raw_node.rs

+            // If we hit a size limit when loading CommittedEntries, clamp
+            // our HardState.Commit to what we're actually returning. This is
+            // also used as our cursor to resume for the next Ready.
+            if let Some(committed_entries) = rd.committed_entries.as_ref() {


What about rd.committed_entries.as_ref().and_then(|c| c.last())?

OMG, it's really beautiful here to use functional pattern.

BusyJay · 2019-02-13T03:24:28Z

tests/integration_cases/test_raw_node.rs

+    cfg.max_committed_size_per_ready = 1024;
+    let storage = new_storage();
+    let mut raw_node = RawNode::new(&cfg, storage.clone(), vec![new_peer(1)]).unwrap();
+    let rd = raw_node.ready();


This three lines seem unnecessary.

Do you mean the 3rd line or all the lines from 121 to 123? I couldn't figure it out why the raw_node should be removed.

BusyJay · 2019-02-13T03:26:59Z

tests/integration_cases/test_raft.rs

@@ -2620,6 +2620,46 @@ fn test_leader_increase_next() {
    }
 }

+#[test]
+fn test_send_append_pagination_with_max_size_per_msg() {


What's the purpose of this case? Maybe you can add a hook to Network to implement a similar case with the original PR.

It ensures the hard_state.commit matches with last committed entry index in Ready if committed_entries overflow the max_committed_size_per_ready.

Hoverbear · 2019-02-20T18:31:20Z

Hi @Fullstop000 have you had a chance to see this feedback? Can I help you at all with it?

Fullstop000 · 2019-02-21T02:13:44Z

@BusyJay @Hoverbear Sorry for the delay. Kind of busy here :(

CLAassistant · 2019-08-19T07:29:39Z

Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
2 out of 3 committers have signed the CLA.

✅ Hoverbear
✅ Fullstop000
❌ Hetian Zhu

Hetian Zhu seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

Hoverbear reviewed Dec 31, 2018

View reviewed changes

src/raft_log.rs Outdated Show resolved Hide resolved

Hoverbear reviewed Dec 31, 2018

View reviewed changes

Hoverbear mentioned this pull request Dec 31, 2018

Update clippy lints. #163

Closed

Hoverbear added this to the 0.5.0 milestone Dec 31, 2018

Hoverbear added the Feature Related to a major feature. label Dec 31, 2018

Hoverbear mentioned this pull request Dec 31, 2018

Fix lints #142

Merged

Hoverbear self-assigned this Jan 4, 2019

Hoverbear reviewed Jan 4, 2019

View reviewed changes

tests/integration_cases/test_raw_node.rs Outdated Show resolved Hide resolved

Hoverbear previously approved these changes Jan 7, 2019

View reviewed changes

Hoverbear requested a review from hicqu January 7, 2019 20:16

BusyJay reviewed Jan 30, 2019

View reviewed changes

src/config.rs Outdated Show resolved Hide resolved

Fullstop000 dismissed Hoverbear’s stale review via 1775a4d January 31, 2019 02:04

Hoverbear previously approved these changes Feb 7, 2019

View reviewed changes

src/raw_node.rs Outdated Show resolved Hide resolved

Hoverbear modified the milestones: 0.5.0, 0.6.0 Feb 11, 2019

Fullstop000 dismissed Hoverbear’s stale review via 7030b24 February 12, 2019 04:06

BusyJay reviewed Feb 13, 2019

View reviewed changes

Hoverbear requested a review from BusyJay March 6, 2019 20:58

Fullstop000 closed this Apr 2, 2020

Fullstop000 force-pushed the master branch from 3f448a0 to b9891b6 Compare April 2, 2020 03:36

Fullstop000 mentioned this pull request Apr 2, 2020

implement committed entries pagination #356

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implement CommittedEntries pagination #161

implement CommittedEntries pagination #161

Fullstop000 commented Dec 26, 2018 •

edited by Hoverbear

Loading

Fullstop000 commented Dec 26, 2018

Hoverbear commented Dec 31, 2018

Hoverbear left a comment

Fullstop000 commented Jan 2, 2019 •

edited

Loading

Hoverbear commented Jan 4, 2019

Fullstop000 commented Jan 5, 2019

Hoverbear left a comment

siddontang commented Jan 30, 2019

Hoverbear left a comment

BusyJay Feb 13, 2019

BusyJay Feb 13, 2019

Fullstop000 Feb 21, 2019 •

edited

Loading

BusyJay Feb 13, 2019

Fullstop000 Feb 21, 2019

BusyJay Feb 13, 2019

Fullstop000 Feb 21, 2019 •

edited

Loading

Hoverbear commented Feb 20, 2019

Fullstop000 commented Feb 21, 2019 •

edited

Loading

CLAassistant commented Aug 19, 2019 •

edited

Loading

implement CommittedEntries pagination #161

implement CommittedEntries pagination #161

Conversation

Fullstop000 commented Dec 26, 2018 • edited by Hoverbear Loading

Fullstop000 commented Dec 26, 2018

Hoverbear commented Dec 31, 2018

Hoverbear left a comment

Choose a reason for hiding this comment

Fullstop000 commented Jan 2, 2019 • edited Loading

Hoverbear commented Jan 4, 2019

Fullstop000 commented Jan 5, 2019

Hoverbear left a comment

Choose a reason for hiding this comment

siddontang commented Jan 30, 2019

Hoverbear left a comment

Choose a reason for hiding this comment

BusyJay Feb 13, 2019

Choose a reason for hiding this comment

BusyJay Feb 13, 2019

Choose a reason for hiding this comment

Fullstop000 Feb 21, 2019 • edited Loading

Choose a reason for hiding this comment

BusyJay Feb 13, 2019

Choose a reason for hiding this comment

Fullstop000 Feb 21, 2019

Choose a reason for hiding this comment

BusyJay Feb 13, 2019

Choose a reason for hiding this comment

Fullstop000 Feb 21, 2019 • edited Loading

Choose a reason for hiding this comment

Hoverbear commented Feb 20, 2019

Fullstop000 commented Feb 21, 2019 • edited Loading

CLAassistant commented Aug 19, 2019 • edited Loading

Fullstop000 commented Dec 26, 2018 •

edited by Hoverbear

Loading

Fullstop000 commented Jan 2, 2019 •

edited

Loading

Fullstop000 Feb 21, 2019 •

edited

Loading

Fullstop000 Feb 21, 2019 •

edited

Loading

Fullstop000 commented Feb 21, 2019 •

edited

Loading

CLAassistant commented Aug 19, 2019 •

edited

Loading