refactor(Ready): add pub getters and make fields private #120

Ryan-Git · 2018-09-09T08:15:18Z

Fixes #90

siddontang

LGTM

Seem we need to upgrade the version too. It breaks the compatibility.

/cc @Hoverbear @BusyJay

BusyJay

It looks very expensive to maintain a read-only ready. And what if users want to advance ready partially?

BusyJay · 2018-09-10T02:53:54Z

src/raw_node.rs

+    /// store/state-machine. These have previously been committed to stable
+    /// store.
+    #[inline]
+    pub fn committed_entries(&self) -> Option<&[Entry]> {


So users have to clone the memory to handle committed entries in other thread now?

BusyJay · 2018-09-10T02:55:44Z

src/raw_node.rs

+    /// If it contains a MsgSnap message, the application MUST report back to raft
+    /// when the snapshot has been received or has failed by calling ReportSnapshot.
+    #[inline]
+    pub fn messages(&self) -> &[Message] {


Channel usually move the message. So a clone seems unavoidable.

Ryan-Git · 2018-09-10T14:22:15Z

@siddontang
shall I bump the version? I saw separate pr to release new versions, thought it should be left for you guys.

@BusyJay
Thanks for the comments. I searched for examples in TiKV and found two
mutable ref:
self.pending_messages = mem::replace(&mut ready.messages, vec![]);
move:
self.send(trans, ready.messages.drain(..), &mut metrics.message)
both of which seem to take and clear the origin message.

I don't have a clear enough understanding of this scenario, @Hoverbear, as the issue owner, what do you think about this?

Hoverbear · 2018-09-10T15:36:34Z

Thanks for taking up the flag on this @Ryan-Git. :)

We will definitely need to include this in a non-patch version since it's an API break.

I think @BusyJay has a very interesting point:

And what if users want to advance ready partially?

@BusyJay, could you expand more on this? I was unaware of this use.

BusyJay · 2018-09-11T06:13:01Z

For example, we may handle committed entries and entries differently so may not advance them all together. Maybe we need a more clear Ready mechanism for this. For example, Use Ready to represent readiness, and use HandleProgress, which is just an example name, to represent the handle progress. So Ready is free to be moved and modify, raft is only required to check the progress.

Hoverbear · 2018-09-11T19:26:17Z

@BusyJay Maybe we should revert the change on these particular fields, and merge this PR, then investigate how to move forward with this solution in another issue?

Ryan-Git · 2018-09-12T08:12:30Z

I could revert changes on messages and committed_entries mentioned above.

Hoverbear · 2018-09-13T22:10:36Z

@Ryan-Git Yeah, let's revert them for now. Thanks so much!

Ryan-Git · 2018-09-14T07:14:34Z

updated

siddontang · 2018-09-16T11:17:40Z

PTAL @BusyJay

Hoverbear · 2018-09-21T19:39:18Z

tests/cases/test_raw_node.rs

@@ -59,21 +59,22 @@ fn conf_change(t: ConfChangeType, node_id: u64) -> ConfChange {
    cc
 }

-fn new_ready(


Maybe we can #[derive(PartialEq)] instead?

Does it mean to add a pub constructor? Otherwise we can't make a Ready from the private fields.

Ah, nevermind. :)

emm.. do you mean to add the constructor or just keep it as it is?

hicqu · 2018-10-09T03:25:37Z

LGTM. Please resolves conflicts.

…cess

Ryan-Git · 2018-10-15T14:06:23Z

LGTM. Please resolves conflicts.

resolved

Hoverbear · 2018-11-01T14:14:54Z

bors r+

120: refactor(Ready): add pub getters and make fields private r=Hoverbear a=Ryan-Git Fixes #90 Co-authored-by: renhongdi <renhongdi@mobike.com> Co-authored-by: Ryan-Git <ryan.hd.ren@gmail.com>

Hoverbear · 2018-11-01T14:54:44Z

@Ryan-Git Thanks, I'm going to drive this to merge. :) I decided to use your PR to test our bors and I have a small hiccup. If you get some notifications you can just ignore them. Sorry about that!

120: refactor(Ready): add pub getters and make fields private r=Hoverbear a=Ryan-Git Fixes #90 Co-authored-by: renhongdi <renhongdi@mobike.com> Co-authored-by: Ryan-Git <ryan.hd.ren@gmail.com> Co-authored-by: A. Hobden <operator@hoverbear.org>

Hoverbear · 2018-11-02T14:25:16Z

bors r+

120: refactor(Ready): add pub getters and make fields private r=Hoverbear a=Ryan-Git Fixes #90 Co-authored-by: renhongdi <renhongdi@mobike.com> Co-authored-by: Ryan-Git <ryan.hd.ren@gmail.com> Co-authored-by: A. Hobden <operator@hoverbear.org>

bors · 2018-11-02T14:41:43Z

Build succeeded

Hoverbear · 2018-11-02T14:42:56Z

🎉 Thank you @Ryan-Git for your patience and hard work, and @bors[bot] for the merge. 😉

Ryan-Git · 2018-11-03T02:35:09Z

@Hoverbear My pleasure :-)

siddontang previously approved these changes Sep 10, 2018

View reviewed changes

BusyJay reviewed Sep 10, 2018

View reviewed changes

Hoverbear added this to the 0.5.0 milestone Sep 11, 2018

Ryan-Git dismissed siddontang’s stale review via f5ab129 September 14, 2018 02:47

Ryan-Git force-pushed the master branch from f5ab129 to 6f9a833 Compare September 14, 2018 02:58

Hoverbear added the Enhancement An improvement to existing code. label Sep 17, 2018

Hoverbear reviewed Sep 21, 2018

View reviewed changes

Ryan-Git added 3 commits October 15, 2018 20:25

refactor(Ready): add pub getters and make fields private

6f37ac4

refactor(Ready): revert committed_entries and messages to have pub ac…

9ad5475

…cess

test(test_raw_node): rebase upstream master

d7fe7f8

Ryan-Git force-pushed the master branch from 43f6d9d to d7fe7f8 Compare October 15, 2018 13:35

Merge branch 'master' into master

e630f0e

Hoverbear approved these changes Nov 1, 2018

View reviewed changes

bors bot added a commit that referenced this pull request Nov 1, 2018

Merge #120

6fb68e4

120: refactor(Ready): add pub getters and make fields private r=Hoverbear a=Ryan-Git Fixes #90 Co-authored-by: renhongdi <renhongdi@mobike.com> Co-authored-by: Ryan-Git <ryan.hd.ren@gmail.com>

This comment has been minimized.

Sign in to view

Merge branch 'master' into master

41e3783

This comment has been minimized.

Sign in to view

tikv deleted a comment from bors bot Nov 2, 2018

bors bot merged commit 41e3783 into tikv:master Nov 2, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(Ready): add pub getters and make fields private #120

refactor(Ready): add pub getters and make fields private #120

Ryan-Git commented Sep 9, 2018

siddontang left a comment

BusyJay left a comment

BusyJay Sep 10, 2018

BusyJay Sep 10, 2018

Ryan-Git commented Sep 10, 2018

Hoverbear commented Sep 10, 2018

BusyJay commented Sep 11, 2018

Hoverbear commented Sep 11, 2018

Ryan-Git commented Sep 12, 2018

Hoverbear commented Sep 13, 2018

Ryan-Git commented Sep 14, 2018

siddontang commented Sep 16, 2018

Hoverbear Sep 21, 2018

Ryan-Git Sep 23, 2018 •

edited

Loading

Hoverbear Sep 25, 2018

Ryan-Git Sep 26, 2018

hicqu commented Oct 9, 2018

Ryan-Git commented Oct 15, 2018

Hoverbear commented Nov 1, 2018

Hoverbear commented Nov 1, 2018

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Hoverbear commented Nov 2, 2018

bors bot commented Nov 2, 2018

Hoverbear commented Nov 2, 2018

Ryan-Git commented Nov 3, 2018

refactor(Ready): add pub getters and make fields private #120

refactor(Ready): add pub getters and make fields private #120

Conversation

Ryan-Git commented Sep 9, 2018

siddontang left a comment

Choose a reason for hiding this comment

BusyJay left a comment

Choose a reason for hiding this comment

BusyJay Sep 10, 2018

Choose a reason for hiding this comment

BusyJay Sep 10, 2018

Choose a reason for hiding this comment

Ryan-Git commented Sep 10, 2018

Hoverbear commented Sep 10, 2018

BusyJay commented Sep 11, 2018

Hoverbear commented Sep 11, 2018

Ryan-Git commented Sep 12, 2018

Hoverbear commented Sep 13, 2018

Ryan-Git commented Sep 14, 2018

siddontang commented Sep 16, 2018

Hoverbear Sep 21, 2018

Choose a reason for hiding this comment

Ryan-Git Sep 23, 2018 • edited Loading

Choose a reason for hiding this comment

Hoverbear Sep 25, 2018

Choose a reason for hiding this comment

Ryan-Git Sep 26, 2018

Choose a reason for hiding this comment

hicqu commented Oct 9, 2018

Ryan-Git commented Oct 15, 2018

Hoverbear commented Nov 1, 2018

Hoverbear commented Nov 1, 2018

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Hoverbear commented Nov 2, 2018

bors bot commented Nov 2, 2018

Build succeeded

Hoverbear commented Nov 2, 2018

Ryan-Git commented Nov 3, 2018

Ryan-Git Sep 23, 2018 •

edited

Loading