raft: add learner and tests. #2491

hicqu · 2017-11-20T05:26:52Z

This PR ports raft learner from etcd.

siddontang · 2017-11-23T01:10:20Z

siddontang · 2017-11-23T01:49:23Z

src/raft/raft.rs

@@ -443,7 +481,11 @@ impl<T: Storage> Raft<T> {
    }

    fn prepare_send_snapshot(&mut self, m: &mut Message, to: u64) -> bool {
-        let pr = self.prs.get_mut(&to).unwrap();
+        let mut pr = match self.prs.get_mut(&to) {


why not using get_mut_progress_by_id here?

siddontang · 2017-11-23T01:51:38Z

src/raft/raft.rs

@@ -498,12 +540,14 @@ impl<T: Storage> Raft<T> {
    }

    fn prepare_send_entries(&mut self, m: &mut Message, to: u64, term: u64, ents: Vec<Entry>) {
-        let pr = self.prs.get_mut(&to).unwrap();
+        let tag = self.tag.clone(); // TODO: recude clone.


why clone here?

siddontang · 2017-11-23T01:55:19Z

src/raft/raft.rs

-            .maybe_update(self.raft_log.last_index());
+
+        {
+            let pr = match self.prs.get_mut(&self.id) {


why not use get_mut_progress_by_id?

BusyJay · 2017-11-23T19:14:39Z

src/raft/raft.rs

@@ -308,8 +322,19 @@ impl<T: Storage> Raft<T> {
            skip_bcast_commit: c.skip_bcast_commit,
            tag: c.tag.to_owned(),
        };
-        for p in peers {
-            r.prs.insert(*p, new_progress(1, r.max_inflight));
+        for &p in peers {


Prefer dereferencing to taking references.

BusyJay · 2017-11-23T19:16:52Z

src/raft/raft.rs

+        self.prs.get(&id).or_else(|| self.learner_prs.get(&id))
+    }
+
+    fn get_mut_progress_by_id(&mut self, id: u64) -> Option<&mut Progress> {


by_id is redundant.

BusyJay · 2017-11-23T19:25:30Z

src/raft/raft.rs

@@ -182,9 +188,12 @@ pub struct Raft<T: Storage> {
    pub max_inflight: usize,
    pub max_msg_size: u64,
    pub prs: FlatMap<u64, Progress>,
+    pub learner_prs: FlatMap<u64, Progress>,


It seems learner_prs and prs are almost always handled together, why not put them into a struct?

@xiang90 suggests separating them. Let's keep the same with etcd.

I'm not suggesting to unify them, but use a struct Prs { prs: map, leaner_prs: map } to manage them all. It's easier to deal with ownership in that way.

hicqu · 2017-11-27T10:51:47Z

@BusyJay @siddontang PTAL.

siddontang · 2017-11-28T01:08:20Z

src/raft/raft.rs

@@ -1836,21 +2003,22 @@ impl<T: Storage> Raft<T> {
    // false.
    // check_quorum_active also resets all recent_active to false.
    fn check_quorum_active(&mut self) -> bool {
-        let mut act = 0;
+        // NOTE: Here is a little different from etcd.


what is the difference?

In etcd, self is always active even if it's a learner. But I think it's always active only if it's a voter but not learner. Is it correct?
See: https://github.com/coreos/etcd/blob/master/raft/raft.go#L1375

But this function can only be called if the peer is a leader.

siddontang · 2017-11-28T01:09:48Z

src/raft/raft.rs

-        // TODO: avoid copy
-        let ids: Vec<_> = self.prs.keys().cloned().collect();
-        for id in ids {
+        for id in self.nodes() {


why use nodes but not mem replace here?

BusyJay · 2017-11-28T04:39:22Z

src/raft/raft.rs

-        for id in ids {
-            if id == self.id {
-                continue;
+        let prs = mem::replace(&mut self.prs, ProgressSet::default());


I think it's a bad idea. What if self.prs is queried before it's reset? It's hard to write correct code.

How about write code like this:

struct Raft { prs: ProgressSet, raft: _Raft, // _Raft is a struct contains all other fields }

So all method of _Raft need pass a &ProgressSet or &mut ProgressSet in.

siddontang · 2017-11-29T06:04:50Z

src/raft/raft.rs

-            if id == self.id {
-                continue;
+        let prs = self.take_prs();
+        for (id, pr) in prs.voters.iter().chain(&prs.learners) {


why not using chain + filter + for_each as above does?

siddontang · 2017-11-29T06:05:00Z

src/raft/raft.rs

-            if id == self.id {
-                continue;
+        let prs = self.take_prs();
+        for (id, pr) in prs.voters.iter().chain(&prs.learners) {


why not using chain + filter + for_each as above does?

siddontang · 2017-11-29T06:08:06Z

PTAL @overvenus

overvenus · 2017-11-30T05:44:43Z

src/raft/raft.rs

        let raft_log = RaftLog::new(store, c.tag.clone());
        let mut peers: &[u64] = &c.peers;
-        if !rs.conf_state.get_nodes().is_empty() {
-            if !peers.is_empty() {
+        let mut learners: &[u64] = &c.learners;


Do we need type declaration?

Yes we need.

overvenus · 2017-11-30T05:47:07Z

src/raft/raft.rs

                // TODO: the peers argument is always nil except in
                // tests; the argument should be removed and these tests should be
                // updated to specify their nodes through a snap
                panic!(
-                    "{} cannot specify both new(peers) and ConfState.Nodes",
+                    "{} cannot specify both new(peers/learners) and ConfState.Nodes",


How about "... and ConfState.(Nodes/Learners)",?
Just keep consistent with etcds[0].

[0] https://github.com/coreos/etcd/pull/8751/files?diff=unified#diff-a8629a0eb6da1c6c3805111f1f1af85eR306

overvenus · 2017-11-30T05:50:40Z

src/raft/raft.rs

@@ -181,10 +187,12 @@ pub struct Raft<T: Storage> {

    pub max_inflight: usize,
    pub max_msg_size: u64,
-    pub prs: FlatMap<u64, Progress>,
+    prs: Option<ProgressSet>,


When does prs become None?

for bcast to avoid cloning ID

overvenus · 2017-11-30T06:18:56Z

src/raft/raft.rs

+                if self.is_learner {
+                    // TODO: learner may need to vote, in case of node down when confchange.
+                    info!(
+                        r#"{} [logterm: {}, index: {}, vote: {}] ignored {:?} from {}


Why not use \ for breaking a long string?

Can't pass rustfmt.

Why? See L1774.

overvenus · 2017-11-30T06:30:01Z

src/raft/raft.rs

@@ -1292,10 +1407,19 @@ impl<T: Storage> Raft<T> {
                );
            }
            MessageType::MsgTransferLeader => {
-                self.handle_transfer_leader(m);
+                let pr = prs.get_mut_progress(m.get_from()).unwrap();


Could from be 0?

CC @BusyJay

can not unless we meet a serious bug.

overvenus · 2017-11-30T07:08:01Z

src/raft/raft.rs

    }

-    pub fn add_node(&mut self, id: u64) {
+    fn add_node_or_learner(&mut self, id: u64, is_learner: bool) {


I think the "node" is a voter, so I suggest rename it to add_voter_or_learner or add_node_or_learner_node.

overvenus · 2017-11-30T07:53:45Z

src/raft/raft.rs

+            .iter_mut()
+            .chain(prs.learners.iter_mut())
+            .filter(|&(id, _)| *id != self_id)
+            .for_each(|(id, ref mut pr)| { self.send_append(*id, pr); });


In my opinion, for loop is straightforward and easier to understand.

overvenus · 2017-11-30T08:10:05Z

src/raft/progress.rs

+    pub learners: FlatMap<u64, Progress>,
+}
+
+impl ProgressSet {


Could you provide a method like fn for_each<F: FnMut(u64, &mut Progress, bool)>(&mut self, func: F)?
So that we do not need to write something like

voter.iter().for_each(do_something); leader.iter().for_each(do_something);

I thinks we don't need that. If we want to apply same closure on voters and learners, we can call voters.iter().chain(&learners).for_each(...) directly.

BusyJay

Can you split this pr into two: change prs to Option and change prs to ProgressSet.

BusyJay · 2017-11-30T12:22:41Z

src/raft/progress.rs

+        progress
+    }
+
+    pub fn check_intersect(&self) {


I think this is redundant if it's checked before inserting.

Now we didn't check before inserting.

If we check it when we insert every node, we need to write the logic twice, once in Raft::new, and once in set_progress.

If we check it before inserting with voters and learners as two Vec, the check will be O(n^2).

So I think call check_intersect after the ProgressSet is stable is better.

we need to write the logic twice, once in Raft::new, and once in set_progress.

Why? What I mean is to provide an insert method and check in it.

If we check it before inserting with voters and learners as two Vec, the check will be O(n^2).

How? I think it should be O(n), where n is total count of nodes and learners. Besides, usually n <= 5, so the time complexity doesn't matter here.

When define a seperate check_intersect method, it requires developer to remember to invoke it every time ProgressSet is updated, which is very easy to make error.

BusyJay · 2017-11-30T12:32:37Z

src/raft/raft.rs

+    }
+
+    pub fn del_progress(&mut self, id: u64) {
+        self.mut_prs().voters.remove(&id);


Should check if return value is None.

BusyJay · 2017-12-11T11:26:33Z

src/raft/progress.rs

+    learners: FlatMap<u64, Progress>,
+}
+
+impl<'a> ProgressSet {


Declare it in method signature instead.

BusyJay · 2017-12-11T11:30:42Z

src/raft/raft.rs

-                // self is always active
-                act += 1;
-                continue;
+        let act = self.mut_prs().iter_mut_voters().fold(0, |act, (&id, pr)| {


I think you can use iter_mut, and check if is_learner to decide whether to update act.

BusyJay · 2017-12-11T11:35:35Z

src/raft/progress.rs

+        }
+    }
+
+    pub fn delete(&mut self, id: u64) -> Option<Progress> {


s/delete/remove/

BusyJay · 2017-12-11T11:41:10Z

src/raft/progress.rs

+            if self.learners.contains_key(&id) {
+                panic!("insert voter {} but already in learners", id);
+            }
+            match self.voters.entry(id) {


Why not check self.voters.insert(id, pr).is_some() instead? Besides it's very strange that insertion of a collection returns the element being inserted.

BusyJay · 2017-12-11T11:45:11Z

src/raft/raft.rs

+    /// learners contains the IDs of all learner nodes (maybe include self if
+    /// the local node is a learner) in the raft cluster.
+    /// learners only receives entries from the leader node. It does not vote
+    /// or promote itself.


What does it mean by not vote or promote itself? Does it mean promote_learner should never be called?

It's ported from etcd. I think it means Learner can't make a proposal to promote itself to Voter. Only Voters can propose to promote a Learner to Voter. I will make the comment more clear.

this means the learner can't vote for other peers or vote for itself.

BusyJay · 2017-12-11T12:01:10Z

src/raft/raft.rs

+                if self.is_learner {
+                    // TODO: learner may need to vote, in case of node down when confchange.
+                    info!(
+                        r#"{} [logterm: {}, index: {}, vote: {}] ignored {:?} from {}


Why? See L1774.

BusyJay · 2017-12-11T12:06:06Z

src/raft/raft.rs

@@ -1177,6 +1217,11 @@ impl<T: Storage> Raft<T> {
    }

    fn handle_transfer_leader(&mut self, m: &Message, pr: &mut Progress) {
+        if self.is_learner {
+            debug!("{} is learner. Ignored transferring leadership", self.id);


Use self.tag instead.

BusyJay · 2017-12-11T12:11:59Z

src/raft/raft.rs

-            // Ignore any redundant addNode calls (which can happen because the
-            // initial bootstrapping entries are applied twice).
+        if self.get_prs().voters().contains_key(&id) {
+            // If is_learner is true, we don't support change Voter to Learner,


Please add a log for the first case.

BusyJay · 2017-12-11T12:16:24Z

src/raft/raft.rs

+        // New progress.
+        let mut pr = new_progress(self.raft_log.last_index() + 1, self.max_inflight);
+        pr.is_learner = is_learner;
+        pr.recent_active = true;


Why set it to true?

It's ported from etcd.

BusyJay · 2017-12-11T12:20:19Z

src/raft/raft.rs

@@ -1802,29 +1885,19 @@ impl<T: Storage> Raft<T> {
        self.pending_conf = false;
    }

-    pub fn set_progress(&mut self, id: u64, matched: u64, next_idx: u64) {


Can you keep the method and add a new arg is_learner? Looks like there are a lot of places using the same pattern.

BusyJay · 2017-12-12T05:05:36Z

src/raft/progress.rs

+    }
+
+    #[allow(needless_lifetimes)]
+    pub fn iter_voters<'a>(&'a self) -> impl Iterator<Item = (&'a u64, &'a Progress)> {


The method is not used anymore, can be removed.

BusyJay · 2017-12-12T05:06:22Z

src/raft/progress.rs

+    }
+
+    #[allow(needless_lifetimes)]
+    pub fn iter_mut_voters<'a>(&'a mut self) -> impl Iterator<Item = (&'a u64, &'a mut Progress)> {


BusyJay · 2017-12-12T05:06:34Z

src/raft/progress.rs

+    }
+
+    #[allow(needless_lifetimes)]
+    pub fn iter_mut_learners<'a>(


BusyJay · 2017-12-12T05:15:04Z

src/raft/raft.rs

+                    info!(
+                        "{} [logterm: {}, index: {}, vote: {}] ignored {:?} from {} \
+                         [logterm: {}, index: {}] at term {}: learner can not vote",
+                        self.id,


Use self.tag instead.

BusyJay · 2017-12-12T05:19:52Z

src/raft/raft.rs

+                if id == self.id {
+                    error!(
+                        "{} can't become learner when restores snapshot [index: {}, term: {}]",
+                        self.id,


BusyJay · 2017-12-12T05:24:17Z

src/raft/raft.rs

@@ -1711,6 +1752,20 @@ impl<T: Storage> Raft<T> {
            return Some(false);
        }

+        if !self.is_learner {


I don't think this is going to work. When a raft node is created, it's not learner by default. So there is no way for a new learner to accept snapshot. So should either set different flag for learner messages or also check if the state machine is initialized.

I think we can pass learners info in Config? So the enw learner can set self.is_learner = true in Raft::new.

When a raft state machine is created via conf change, there is no way to tell if it's a learner or a voter in current implementations.

BusyJay · 2017-12-12T05:29:01Z

src/raft/raft.rs

+            return;
+        }
+
+        if !is_learner && self.mut_prs().promote_learner(id) {


If promotion succeeds, does its active flag need to be set?

BusyJay · 2017-12-12T05:30:09Z

src/raft/raft.rs

        let last_index = self.raft_log.last_index();
-        self.set_progress(id, 0, last_index + 1);
+        self.set_progress(id, 0, last_index + 1, is_learner);


Should it be ignored if is_learner is true, and self.get_prs().learners().contains_key(&id)?

BusyJay · 2017-12-12T05:33:21Z

src/raft/raft.rs

    }

-    pub fn add_node(&mut self, id: u64) {
+    fn add_voter_or_learner(&mut self, id: u64, is_learner: bool) {


Can you keep the logic in this function be same as etcd? The rewrite have introduced several problems already.

BusyJay · 2017-12-12T05:36:24Z

src/raft/raft.rs

        self.prs = Some(prs);
    }

-    pub fn get_prs(&self) -> &FlatMap<u64, Progress> {
+    pub fn get_prs(&self) -> &ProgressSet {


s/get_prs/prs/

hicqu · 2017-12-12T12:11:33Z

@BusyJay PTAL.

BusyJay · 2017-12-13T11:00:22Z

tests/raft/test_raft.rs

@@ -1427,7 +1433,11 @@ fn test_commit() {

        let mut sm = new_test_raft(1, vec![1], 5, 1, store);
        for (j, &v) in matches.iter().enumerate() {
-            sm.set_progress(j as u64 + 1, v, v + 1);
+            let pr = new_progress(ProgressState::default(), v, v + 1, 0, sm.max_inflight);


Why not use set_progress?

BusyJay · 2017-12-13T11:48:41Z

src/raft/progress.rs

+    }
+
+    pub fn promote_learner(&mut self, id: u64) {
+        match self.learners.entry(id) {


Why not use remove?

BusyJay · 2017-12-13T11:56:56Z

tests/raft/test_raft.rs

-                prs.insert(
-                    *id,
-                    Progress {
+                if prs.learners().get(id).is_some() {


contains_key?

BusyJay · 2017-12-13T11:57:30Z

tests/raft/test_raft.rs

@@ -584,16 +589,17 @@ fn test_progress_resume_by_heartbeat_resp() {
    let mut raft = new_test_raft(1, vec![1, 2], 5, 1, new_storage());
    raft.become_candidate();
    raft.become_leader();
-    raft.mut_prs().get_mut(&2).unwrap().paused = true;
+    raft.mut_prs().get_mut(2).unwrap().paused = true;
+    raft.mut_prs().get_mut(2).unwrap().paused = true;


Why set twice?

BusyJay · 2017-12-13T12:05:13Z

tests/raft/test_raft.rs

+}
+
+#[test]
+fn test_learner_election_timeout() {


Please keep the comment for the test case.

BusyJay · 2017-12-13T12:12:20Z

tests/raft/test_raft.rs

+    // Promote n2 from learner to follower.
+    network.peers.get_mut(&1).unwrap().add_node(2);
+    network.peers.get_mut(&2).unwrap().add_node(2);
+    assert_eq!(network.peers[&2].state, StateRole::Follower);


Should check if it's learner.

In etcd it is

if n2.isLearner { t.Error("peer 2 is learner, want not") }

I think they are same because n2 must not be leader?

Yes, if there is no bug. Unfortunately you can't assume there is no bug in tests.

BusyJay · 2017-12-13T12:15:42Z

tests/raft/test_raft.rs

+    n2.set_randomized_election_timeout(timeout);
+
+    for _ in 0..timeout {
+        n1.tick();


It's meaningless to tick when it's not in a network.

BusyJay · 2017-12-13T12:16:14Z

tests/raft/test_raft.rs

+
+    let timeout = n1.get_election_timeout();
+    n1.set_randomized_election_timeout(timeout);
+    n2.set_randomized_election_timeout(timeout);


BusyJay · 2017-12-13T12:18:24Z

tests/raft/test_raft.rs

+    network.send(vec![msg]);
+
+    assert_eq!(
+        network.peers.get_mut(&2).unwrap().raft_log.committed,


Should check node 1 too.

BusyJay · 2017-12-13T12:19:55Z

tests/raft/test_raft.rs

+
+#[test]
+fn test_restore_with_learner() {
+    let mut s = Snapshot::new();


Why not use new_snapshot?

hicqu · 2017-12-14T05:52:38Z

@BusyJay PTAL thanks.

BusyJay

There are still several places can be replaced by new_snapshot and new_message. Also please keep the comments of all ported test cases.

BusyJay · 2017-12-14T06:44:25Z

src/raft/raft.rs

+    ///
+    /// The flag will be true if all of `Config.peers` and `Config.learners`
+    /// and `peers/learners` from `PeerStorage` are empty.
+    initialized: bool,


Why not just check self.peers and self.learners at L1769?

BusyJay · 2017-12-14T06:58:03Z

src/raft/raft.rs

@@ -1711,6 +1766,20 @@ impl<T: Storage> Raft<T> {
            return Some(false);
        }

+        if self.initialized && !self.is_learner {


We don't behave the same as Etcd now. I think it's reasonable but not sure if it's a good way. Another way is to add an additional flag to the messages sent to learners. Any ideas? For the reason of the changes see discussion at #2491 (comment).

/cc @siddontang @xiang90

hicqu · 2017-12-15T03:53:31Z

@BusyJay PTAL thanks.

BusyJay · 2017-12-15T04:31:55Z

LGTM

hicqu · 2017-12-15T04:37:57Z

@siddontang PTAL again, thanks.

hicqu · 2017-12-15T05:54:40Z

/run-all-tests

siddontang · 2017-12-15T08:24:14Z

PTAL @overvenus

overvenus · 2017-12-15T11:08:09Z

src/raft/progress.rs

+        progress
+    }
+
+    // We need explicit lifetime here because of a compiler bug.


Please add the reference.

overvenus

LGTM

raft: add learner and tests.

9801e56

hicqu requested review from BusyJay, siddontang and overvenus November 20, 2017 05:27

fix CI.

7d0acca

siddontang reviewed Nov 23, 2017

View reviewed changes

BusyJay reviewed Nov 23, 2017

View reviewed changes

hicqu added 3 commits November 27, 2017 17:19

fix code.

3d50e70

address comments.

f943ab4

Merge branch 'master' into raft-learner

45954d1

fix ci.

fe210a1

siddontang reviewed Nov 28, 2017

View reviewed changes

address comments.

88e357a

BusyJay reviewed Nov 28, 2017

View reviewed changes

hicqu added 3 commits November 28, 2017 16:45

todo: fix tests.

7793b4f

address comments.

4ddc169

Merge branch 'master' into raft-learner

25acf62

siddontang reviewed Nov 29, 2017

View reviewed changes

address comments.

5158f21

overvenus reviewed Nov 30, 2017

View reviewed changes

address comments.

758714e

BusyJay reviewed Nov 30, 2017

View reviewed changes

Merge branch 'master' into raft-learner

94ebfdf

BusyJay reviewed Dec 11, 2017

View reviewed changes

hicqu added 2 commits December 12, 2017 12:53

address comments.

128d128

fix ci.

bf0d9e0

BusyJay reviewed Dec 12, 2017

View reviewed changes

hicqu and others added 2 commits December 12, 2017 14:44

address comments.

ac008cb

Merge branch 'master' into raft-learner

396563d

BusyJay reviewed Dec 13, 2017

View reviewed changes

address comments.

fa955e2

hicqu force-pushed the raft-learner branch from 36bf95f to fa955e2 Compare December 14, 2017 05:51

Merge branch 'master' into raft-learner

4372b9f

BusyJay reviewed Dec 14, 2017

View reviewed changes

hicqu added 3 commits December 14, 2017 15:36

address comments

d2e6b2e

Merge branch 'raft-learner' of github.com:hicqu/tikv into raft-learner

96f887c

Merge branch 'master' into raft-learner

e03d640

overvenus reviewed Dec 15, 2017

View reviewed changes

src/raft/progress.rs

progress

}

// We need explicit lifetime here because of a compiler bug.

Copy link

Member

overvenus Dec 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add the reference.

hicqu added 2 commits December 18, 2017 11:19

Merge branch 'master' into raft-learner

21f38d7

address comments.

3474177

overvenus approved these changes Dec 18, 2017

View reviewed changes

hicqu merged commit d780a5f into tikv:master Dec 18, 2017

hicqu deleted the raft-learner branch December 18, 2017 05:15

siddontang mentioned this pull request Dec 19, 2017

raft as crate #2594

Closed

sticnarf pushed a commit to sticnarf/tikv that referenced this pull request Oct 27, 2019

raft: add learner and tests. (tikv#2491)

9a57720

raft: add learner and tests. #2491

raft: add learner and tests. #2491

Conversation

hicqu commented Nov 20, 2017

siddontang commented Nov 23, 2017

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

hicqu commented Nov 27, 2017

Choose a reason for hiding this comment

hicqu Nov 28, 2017 • 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

siddontang commented Nov 29, 2017

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

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

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

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

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

hicqu commented Dec 12, 2017

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

hicqu Dec 14, 2017 • edited Loading

hicqu Nov 28, 2017 •

edited

Loading

hicqu Dec 14, 2017 •

edited

Loading