Mike Percy has posted comments on this change. Change subject: KUDU-1034 client does not failover due to timeout ......................................................................
Patch Set 1: (3 comments) http://gerrit.cloudera.org:8080/#/c/6924/1/src/kudu/integration-tests/raft_consensus-itest.cc File src/kudu/integration-tests/raft_consensus-itest.cc: Line 420: // Test that a client fails over when the leader times out. > After some consideration I think it's also a good place for the test since I think client_failover-itest.cc is a better home for this test, since it's a good fit with what we're testing (this is testing a client failover scenario) and uses basically the same infrastructure as this test (ExternalMiniCluster). PS1, Line 450: for (int i = 0; i < 1000; i++) { : if (workload.rows_inserted() >= rows_target) break; : SleepFor(MonoDelta::FromMilliseconds(10)); : } > I take it back: after some consideration I think that in the scope of KUDU- The client should respect KuduSession.setTimeout() and I believe that is the timeout value that is triggering here (albeit triggering in the resulting RPC call), so unless we want to shorten the RPC timeout to be less than the session timeout (probably not a good idea without careful consideration) then the client has no choice but to respond to the user application with a timeout error. http://gerrit.cloudera.org:8080/#/c/6924/1/src/kudu/rpc/retriable_rpc.h File src/kudu/rpc/retriable_rpc.h: Line 217: server_picker_->MarkServerFailed(server, result.status); OK now I remember the issue I had when looking at this issue previously. I the problem with using MarkServerFailed() here is that ALL replicas on the server are considered failed, not just this one. The client needs a concept of only a single replica failed and IIRC that's not expressible in the client failover codebase today. -- To view, visit http://gerrit.cloudera.org:8080/6924 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Icfcece485e4053d921ffdc865612b3e7b9a992a3 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
