check raftcmd term. #1328

disksing · 2016-11-21T03:07:30Z

~~Still working on detailed test.~~

ngaut · 2016-11-21T07:56:42Z

src/server/kv.rs

@@ -298,7 +298,10 @@ impl StoreHandler {
        resp.set_cmd_gc_resp(gc);
    }

-    pub fn on_request(&self, req: Request, on_resp: OnResponse) -> Result<()> {
+    pub fn on_request(&self, mut req: Request, on_resp: OnResponse) -> Result<()> {
+        // Go client may set it to 0 if using gogoprotobuf.


Why go client needs term?

Context is sent from client.

disksing · 2016-11-22T13:33:41Z

Ready to review.
/cc @BusyJay @siddontang @ngaut @zhangjinpeng1987

siddontang · 2016-11-22T13:38:51Z

src/server/kv.rs

-    pub fn on_request(&self, req: Request, on_resp: OnResponse) -> Result<()> {
+    pub fn on_request(&self, mut req: Request, on_resp: OnResponse) -> Result<()> {
+        // Go client may set it to 0 if using gogoprotobuf.
+        req.mut_context().clear_term();


why clear term here?

gogoprotobuf will always set it, then we cannot use has_term(). See pingcap/kvproto#97

siddontang · 2016-11-24T03:38:50Z

src/storage/engine/raftkv.rs

@@ -95,7 +95,7 @@ pub struct RaftKv<S: RaftStoreRouter + 'static> {

 enum CmdRes {
    Resp(Vec<Response>),
-    Snap(RegionSnapshot),
+    Snap((RegionSnapshot, u64)),


I prefer using a field structure instead of tuple here.

siddontang · 2016-11-24T03:39:25Z

src/storage/engine/raftkv.rs

@@ -237,17 +240,23 @@ impl<S: RaftStoreRouter> Engine for RaftKv<S> {
        ASYNC_REQUESTS_COUNTER_VEC.with_label_values(&["snapshot", "all"]).inc();
        let req_timer = ASYNC_REQUESTS_DURATIONS_VEC.with_label_values(&["snapshot"]).start_timer();

+        let mut ctx2 = ctx.clone();


does clone hurt the performance?

disksing · 2016-11-24T08:28:36Z

@siddontang PTAL. I've add struct CbContext to carry data that engines need to pass back to upper level.

siddontang · 2016-11-24T09:17:42Z

src/storage/txn/scheduler.rs

            let ctx = &mut self.cmd_ctxs.get_mut(&cid).unwrap();
            assert_eq!(ctx.cid, cid);
            ctx.cmd.take().unwrap()
        };
+        if let Some(term) = cb_ctx.term {


seem you can use cmd.set_context(cb_ctx) directly?

CbContext is newly defined which is not the same type as Context.

BusyJay · 2016-11-25T07:14:22Z

src/server/coprocessor/endpoint.rs

@@ -147,7 +147,7 @@ impl BatchRunnable<Task> for Host {
            let id = self.last_req_id;
            let sched = self.sched.clone();
            if let Err(e) = self.engine.async_snapshot(reqs[0].req.get_context(),
-                                                       box move |res| {
+                                                       box move |(_cb_ctx, res)| {


Use _ directly.

BusyJay · 2016-11-25T07:15:50Z

src/raftstore/store/store.rs

@@ -1100,6 +1100,12 @@ impl<T: Transport, C: PdClient> Store<T, C> {
            return Err(box_err!("mismatch peer id {} != {}", peer.peer_id(), peer_id));
        }

+        let header = msg.get_header();
+        // If header's term is 2 verions behind current term, leadership may have been changed away.
+        if header.has_term() && peer.term() > header.get_term() + 1 {


Use get_term() != 0 && ... instead.

BusyJay · 2016-11-25T07:26:06Z

src/storage/engine/mod.rs

-pub type Callback<T> = Box<FnBox(Result<T>) + Send>;
+pub type Callback<T> = Box<FnBox((CbContext, Result<T>)) + Send>;
+
+pub struct CbContext {


Why define another CbContext? It change the API a lot. We have many context already, add another one just makes things complicated and confusing. I think it's ugly and unnecessary.

First of all, old API does not support underlying engine to return more data other than Result, we need to change it no matter we reuse Context or not.
In addition, the content of the two Context are different, and subsequent changes will make them more different. I see no reason to reuse it.

I'm not saying reuse the Context. You can check the Result to get the actual term.

In fact, my next PR need it return extra data no matter the result is Ok or Err.

disksing · 2016-11-29T05:38:23Z

PTAL @siddontang @BusyJay
I see no better way other than add the new context. We might rethink it later.

siddontang · 2016-11-29T11:07:59Z

LGTM
PTAL @zhangjinpeng1987

zhangjinpeng87 · 2016-11-29T11:20:31Z

src/raftstore/store/store.rs

@@ -1100,6 +1100,12 @@ impl<T: Transport, C: PdClient> Store<T, C> {
            return Err(box_err!("mismatch peer id {} != {}", peer.peer_id(), peer_id));
        }

+        let header = msg.get_header();
+        // If header's term is 2 verions behind current term, leadership may have been changed away.
+        if header.get_term() > 0 && peer.term() > header.get_term() + 1 {


Why not use peer.term() != hader.get_term() directly?

term may increase without leader changed, we have already checked it is leader, so this case is okay.

zhangjinpeng87 · 2016-11-30T04:55:55Z

LGTM

# Conflicts: # Cargo.lock

BusyJay · 2016-11-30T12:46:17Z

src/raftstore/store/peer.rs

-        let leader = self.get_peer_from_cache(self.leader_id());
-        let not_leader = Error::NotLeader(self.region_id, leader);
-        let resp = cmd_resp::err_resp(not_leader, cmd.uuid, self.term());
+    /// Call the callback of `cmd` that is stale.


Please explain in the comment what does stale mean.

BusyJay · 2016-11-30T12:48:49Z

src/storage/engine/mod.rs

@@ -33,7 +33,17 @@ pub const TEMP_DIR: &'static str = "";
 const SEEK_BOUND: usize = 30;
 const DEFAULT_TIMEOUT_SECS: u64 = 5;

-pub type Callback<T> = Box<FnBox(Result<T>) + Send>;
+pub type Callback<T> = Box<FnBox((CbContext, Result<T>)) + Send>;


Why use tuple?

wait_op!() will be broken if not use tuple.

BusyJay · 2016-11-30T12:51:53Z

src/storage/engine/raftkv.rs

@@ -149,6 +155,9 @@ impl<S: RaftStoreRouter> RaftKv<S> {
        header.set_region_epoch(ctx.get_region_epoch().clone());
        header.set_uuid(Uuid::new_v4().as_bytes().to_vec());
        header.set_read_quorum(ctx.get_read_quorum());
+        if ctx.has_term() {


Use get_term() != 0 instead.

BusyJay · 2016-11-30T13:08:55Z

tests/storage/test_raft_storage.rs

+    assert!(engine.write(&ctx, vec![]).is_err());
+    // Term not match.
+    cluster.must_transfer_leader(region.get_id(), peers[0].clone());
+    assert!(engine.write(&ctx, vec![]).is_err());


How do you know the error due to term not matched?

disksing · 2016-12-01T03:28:16Z

PTAL @BusyJay

BusyJay · 2016-12-01T05:17:47Z

tests/storage/test_raft_storage.rs

+    cluster.must_transfer_leader(region.get_id(), peers[0].clone());
+    block.store(false, Ordering::SeqCst);
+
+    rx.recv().unwrap();


Use recv_timeout instead.

BusyJay · 2016-12-01T05:20:12Z

src/raftstore/errors.rs

@@ -114,6 +114,9 @@ quick_error!{
            description("region is stale")
            display("StaleEpoch {}", msg)
        }
+        StaleCommand {


What will client do when encounter StaleCommand? Will it backoff?

Yes. Backoff then retry.

I don't think backoff is necessary here, because the new leader should be elected, it can retry immediately.

BusyJay · 2016-12-01T07:43:38Z

LGTM

disksing added 2 commits November 17, 2016 16:56

add test.

b56117f

scheduler, raftstore: check term.

b00013a

ngaut reviewed Nov 21, 2016

View reviewed changes

disksing added 4 commits November 21, 2016 16:37

accept request if term is only 1 version behind.

e29c408

add test.

298ad1c

fix CI.

e4b6639

StaleTerm -> StaleCommand

a9baf14

disksing changed the title ~~[DNM] check raftcmd term.~~ check raftcmd term. Nov 22, 2016

siddontang reviewed Nov 22, 2016

View reviewed changes

siddontang reviewed Nov 24, 2016

View reviewed changes

disksing added 2 commits November 24, 2016 15:17

storage/engine: add CbContext.

35bbe76

*: fix CI.

20dfddf

siddontang reviewed Nov 24, 2016

View reviewed changes

BusyJay reviewed Nov 25, 2016

View reviewed changes

address comment.

47591f6

zhangjinpeng87 reviewed Nov 29, 2016

View reviewed changes

disksing added 2 commits November 30, 2016 13:24

Merge branch 'master' into disksing/snapshot-term

9c53a1c

# Conflicts: # Cargo.lock

resolve conflicts.

1f1c5db

BusyJay reviewed Nov 30, 2016

View reviewed changes

address comment.

bae8d06

BusyJay reviewed Dec 1, 2016

View reviewed changes

address comment.

02f62bd

disksing merged commit d332929 into master Dec 2, 2016

disksing deleted the disksing/snapshot-term branch December 2, 2016 02:12

BusyJay mentioned this pull request Dec 3, 2016

tests: retry when get StaleCommand error #1387

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

check raftcmd term. #1328

check raftcmd term. #1328

disksing commented Nov 21, 2016 •

edited

Loading

ngaut Nov 21, 2016

disksing Nov 21, 2016

disksing commented Nov 22, 2016

siddontang Nov 22, 2016

disksing Nov 22, 2016

siddontang Nov 24, 2016

siddontang Nov 24, 2016

disksing commented Nov 24, 2016

siddontang Nov 24, 2016

disksing Nov 24, 2016

BusyJay Nov 25, 2016

BusyJay Nov 25, 2016

BusyJay Nov 25, 2016

disksing Nov 25, 2016

BusyJay Nov 25, 2016

disksing Nov 28, 2016

disksing commented Nov 29, 2016

siddontang commented Nov 29, 2016

zhangjinpeng87 Nov 29, 2016

disksing Nov 29, 2016

zhangjinpeng87 commented Nov 30, 2016

BusyJay Nov 30, 2016

BusyJay Nov 30, 2016

disksing Dec 1, 2016

BusyJay Nov 30, 2016

BusyJay Nov 30, 2016

disksing commented Dec 1, 2016

BusyJay Dec 1, 2016

BusyJay Dec 1, 2016

disksing Dec 1, 2016

BusyJay Dec 1, 2016

BusyJay commented Dec 1, 2016

check raftcmd term. #1328

check raftcmd term. #1328

Conversation

disksing commented Nov 21, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

disksing commented Nov 22, 2016

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

disksing commented Nov 24, 2016

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

disksing commented Nov 29, 2016

siddontang commented Nov 29, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhangjinpeng87 commented Nov 30, 2016

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

disksing commented Dec 1, 2016

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 commented Dec 1, 2016

disksing commented Nov 21, 2016 •

edited

Loading