You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Alexey Serbin (Code Review)" <ge...@cloudera.org> on 2017/05/19 16:46:27 UTC

[kudu-CR] KUDU-1580 retry tserver RPC if negotiation times out 

Alexey Serbin has uploaded a new change for review.

  http://gerrit.cloudera.org:8080/6926

Change subject: KUDU-1580 retry tserver RPC if negotiation times out
......................................................................

KUDU-1580 retry tserver RPC if negotiation times out

This patch addresses KUDU-1580, i.e. with this patch the Kudu C++ client
retries an RPC with other tablet replica if the connection negotiation
with current replica timed out.

Added new integration test to cover the updated client's behavior.

Change-Id: Icee8bf4978365a23d6627e7bc411b63f53540a3b
---
M src/kudu/client/batcher.cc
M src/kudu/integration-tests/client_failover-itest.cc
M src/kudu/rpc/connection.cc
M src/kudu/rpc/reactor.cc
4 files changed, 141 insertions(+), 10 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/26/6926/1
-- 
To view, visit http://gerrit.cloudera.org:8080/6926
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: Icee8bf4978365a23d6627e7bc411b63f53540a3b
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>

[kudu-CR] KUDU-1580 retry tserver RPC if negotiation times out

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.

Change subject: KUDU-1580 retry tserver RPC if negotiation times out
......................................................................


Patch Set 6:

(4 comments)

I'm not sure what the purpose is of NEGOTIATION_TIMED_OUT in the current patch.

http://gerrit.cloudera.org:8080/#/c/6926/4/src/kudu/rpc/outbound_call.cc
File src/kudu/rpc/outbound_call.cc:

Line 314:   const MonoDelta timeout = controller_->timeout();
did you mean const& ?


http://gerrit.cloudera.org:8080/#/c/6926/6/src/kudu/rpc/outbound_call.cc
File src/kudu/rpc/outbound_call.cc:

Line 330:   return state_ == TIMED_OUT;
return state_ == TIMED_OUT || state_ == NEGOTIATION_TIMED_OUT ?


http://gerrit.cloudera.org:8080/#/c/6926/4/src/kudu/rpc/outbound_call.h
File src/kudu/rpc/outbound_call.h:

PS4, Line 124: remove
remote


http://gerrit.cloudera.org:8080/#/c/6926/6/src/kudu/rpc/rpc_introspection.proto
File src/kudu/rpc/rpc_introspection.proto:

Line 43:     NEGOTIATION_TIMED_OUT = 7;
I don't really understand why these are defined both here and in outbound_call.h, can't we just use these ones everywhere?


-- 
To view, visit http://gerrit.cloudera.org:8080/6926
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Icee8bf4978365a23d6627e7bc411b63f53540a3b
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1580 retry tserver RPC if negotiation times out

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.

Change subject: KUDU-1580 retry tserver RPC if negotiation times out
......................................................................


Patch Set 7:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/6926/7/src/kudu/rpc/outbound_call.cc
File src/kudu/rpc/outbound_call.cc:

Line 331:     case NEGOTIATION_TIMED_OUT:       // fall-through
It is possible to assert on this in a test somewhere?


http://gerrit.cloudera.org:8080/#/c/6926/6/src/kudu/rpc/rpc_introspection.proto
File src/kudu/rpc/rpc_introspection.proto:

Line 43:     NEGOTIATION_TIMED_OUT = 7;
> I suspect the idea was to limit dependencies between components and have mo
Hmm ok. I suspect there is no great reason TBH but we can defer on that


-- 
To view, visit http://gerrit.cloudera.org:8080/6926
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Icee8bf4978365a23d6627e7bc411b63f53540a3b
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1580 retry tserver RPC if negotiation times out

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.

Change subject: KUDU-1580 retry tserver RPC if negotiation times out
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6926/1/src/kudu/rpc/connection.cc
File src/kudu/rpc/connection.cc:

PS1, Line 170:       const Status reported_status = status.IsTimedOut()
             :           ? Status::Incomplete("RPC is not sent due to connection error",
             :                                status.ToString())
             :           : status;
             :  
> This sounds like a better idea.  Thanks!
Yea, changing Status is unfortunately a big project (has ABI compatibility concerns, too) and harder to figure out how to get all possible error codes into a single global namespace.


-- 
To view, visit http://gerrit.cloudera.org:8080/6926
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Icee8bf4978365a23d6627e7bc411b63f53540a3b
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1580 retry tserver RPC if negotiation times out

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged.

Change subject: KUDU-1580 retry tserver RPC if negotiation times out
......................................................................


KUDU-1580 retry tserver RPC if negotiation times out

This patch addresses KUDU-1580, i.e. with this patch the Kudu C++ client
retries an RPC with other tablet replica if the connection negotiation
with current replica timed out.

Added new integration test to cover the updated client's behavior.

Change-Id: Icee8bf4978365a23d6627e7bc411b63f53540a3b
Reviewed-on: http://gerrit.cloudera.org:8080/6926
Reviewed-by: Mike Percy <mp...@apache.org>
Tested-by: Kudu Jenkins
---
M src/kudu/client/batcher.cc
M src/kudu/integration-tests/client_failover-itest.cc
M src/kudu/integration-tests/raft_consensus-itest.cc
M src/kudu/rpc/connection.cc
M src/kudu/rpc/outbound_call.cc
M src/kudu/rpc/outbound_call.h
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/rpc_controller.cc
M src/kudu/rpc/rpc_controller.h
M src/kudu/rpc/rpc_introspection.proto
12 files changed, 265 insertions(+), 39 deletions(-)

Approvals:
  Mike Percy: Looks good to me, approved
  Kudu Jenkins: Verified



-- 
To view, visit http://gerrit.cloudera.org:8080/6926
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: Icee8bf4978365a23d6627e7bc411b63f53540a3b
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] KUDU-1580 retry tserver RPC if negotiation times out

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/6926

to look at the new patch set (#2).

Change subject: KUDU-1580 retry tserver RPC if negotiation times out
......................................................................

KUDU-1580 retry tserver RPC if negotiation times out

This patch addresses KUDU-1580, i.e. with this patch the Kudu C++ client
retries an RPC with other tablet replica if the connection negotiation
with current replica timed out.

Added new integration test to cover the updated client's behavior.

Change-Id: Icee8bf4978365a23d6627e7bc411b63f53540a3b
---
M src/kudu/client/batcher.cc
M src/kudu/integration-tests/client_failover-itest.cc
M src/kudu/integration-tests/raft_consensus-itest.cc
M src/kudu/rpc/connection.cc
M src/kudu/rpc/reactor.cc
5 files changed, 141 insertions(+), 15 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/26/6926/2
-- 
To view, visit http://gerrit.cloudera.org:8080/6926
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Icee8bf4978365a23d6627e7bc411b63f53540a3b
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] KUDU-1580 retry tserver RPC if negotiation times out

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.

Change subject: KUDU-1580 retry tserver RPC if negotiation times out
......................................................................


Patch Set 9: Code-Review+2

-- 
To view, visit http://gerrit.cloudera.org:8080/6926
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Icee8bf4978365a23d6627e7bc411b63f53540a3b
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] KUDU-1580 retry tserver RPC if negotiation times out

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.

Change subject: KUDU-1580 retry tserver RPC if negotiation times out
......................................................................


Patch Set 7:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6926/7/src/kudu/rpc/outbound_call.cc
File src/kudu/rpc/outbound_call.cc:

Line 331:     case NEGOTIATION_TIMED_OUT:       // fall-through
> It is possible to assert on this in a test somewhere?
What exactly do we want to assert?


-- 
To view, visit http://gerrit.cloudera.org:8080/6926
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Icee8bf4978365a23d6627e7bc411b63f53540a3b
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1580 retry tserver RPC if negotiation times out

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.

Change subject: KUDU-1580 retry tserver RPC if negotiation times out
......................................................................


Patch Set 8: Code-Review+2

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6926/7/src/kudu/rpc/outbound_call.cc
File src/kudu/rpc/outbound_call.cc:

Line 331:     case NEGOTIATION_TIMED_OUT:       // fall-through
> What exactly do we want to assert?
Hmm it looks like it's only actually used by an internal runtime assertion. I guess it's fine.


-- 
To view, visit http://gerrit.cloudera.org:8080/6926
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Icee8bf4978365a23d6627e7bc411b63f53540a3b
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1580 retry tserver RPC if negotiation times out

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/6926

to look at the new patch set (#9).

Change subject: KUDU-1580 retry tserver RPC if negotiation times out
......................................................................

KUDU-1580 retry tserver RPC if negotiation times out

This patch addresses KUDU-1580, i.e. with this patch the Kudu C++ client
retries an RPC with other tablet replica if the connection negotiation
with current replica timed out.

Added new integration test to cover the updated client's behavior.

Change-Id: Icee8bf4978365a23d6627e7bc411b63f53540a3b
---
M src/kudu/client/batcher.cc
M src/kudu/integration-tests/client_failover-itest.cc
M src/kudu/integration-tests/raft_consensus-itest.cc
M src/kudu/rpc/connection.cc
M src/kudu/rpc/outbound_call.cc
M src/kudu/rpc/outbound_call.h
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/rpc_controller.cc
M src/kudu/rpc/rpc_controller.h
M src/kudu/rpc/rpc_introspection.proto
12 files changed, 265 insertions(+), 39 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/26/6926/9
-- 
To view, visit http://gerrit.cloudera.org:8080/6926
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Icee8bf4978365a23d6627e7bc411b63f53540a3b
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] KUDU-1580 retry tserver RPC if negotiation times out

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.

Change subject: KUDU-1580 retry tserver RPC if negotiation times out
......................................................................


Patch Set 6:

(4 comments)

> (4 comments)
 > 
 > I'm not sure what the purpose is of NEGOTIATION_TIMED_OUT in the
 > current patch.

The new NEGOTIATION_TIMED_OUT state is to mark an outbound call which timed out on connection negotiation.  It's different from timing out on sending data for RPC and awaiting for response.

Basically, the idea is to distinguish between errors which happen during connection negotiation and errors that happen while performing a RPC over already established connection.

http://gerrit.cloudera.org:8080/#/c/6926/4/src/kudu/rpc/outbound_call.cc
File src/kudu/rpc/outbound_call.cc:

Line 314:   const MonoDelta timeout = controller_->timeout();
> did you mean const& ?
not quite so: RpcController::timeout() returns by value, and MonoDelta reference would be the same size as the object itself, so no point to have a reference here.  I added 'const' just for readability, to allow the reader to understand this is not going to change in the scope.


http://gerrit.cloudera.org:8080/#/c/6926/6/src/kudu/rpc/outbound_call.cc
File src/kudu/rpc/outbound_call.cc:

Line 330:   return state_ == TIMED_OUT;
> return state_ == TIMED_OUT || state_ == NEGOTIATION_TIMED_OUT ?
I thought about that but was not sure it would work due to the  way it's used in CallTransferCallbacks::NotifyTransferFinished

I'll give it a second look.


http://gerrit.cloudera.org:8080/#/c/6926/4/src/kudu/rpc/outbound_call.h
File src/kudu/rpc/outbound_call.h:

PS4, Line 124: remove
> remote
Done


http://gerrit.cloudera.org:8080/#/c/6926/6/src/kudu/rpc/rpc_introspection.proto
File src/kudu/rpc/rpc_introspection.proto:

Line 43:     NEGOTIATION_TIMED_OUT = 7;
> I don't really understand why these are defined both here and in outbound_c
I suspect the idea was to limit dependencies between components and have more flexibility for states of OutboundCall.  I just followed the existing practice here.


-- 
To view, visit http://gerrit.cloudera.org:8080/6926
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Icee8bf4978365a23d6627e7bc411b63f53540a3b
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1580 retry tserver RPC if negotiation times out

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/6926

to look at the new patch set (#8).

Change subject: KUDU-1580 retry tserver RPC if negotiation times out
......................................................................

KUDU-1580 retry tserver RPC if negotiation times out

This patch addresses KUDU-1580, i.e. with this patch the Kudu C++ client
retries an RPC with other tablet replica if the connection negotiation
with current replica timed out.

Added new integration test to cover the updated client's behavior.

Change-Id: Icee8bf4978365a23d6627e7bc411b63f53540a3b
---
M src/kudu/client/batcher.cc
M src/kudu/integration-tests/client_failover-itest.cc
M src/kudu/integration-tests/raft_consensus-itest.cc
M src/kudu/rpc/connection.cc
M src/kudu/rpc/outbound_call.cc
M src/kudu/rpc/outbound_call.h
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/rpc_controller.cc
M src/kudu/rpc/rpc_controller.h
M src/kudu/rpc/rpc_introspection.proto
12 files changed, 264 insertions(+), 39 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/26/6926/8
-- 
To view, visit http://gerrit.cloudera.org:8080/6926
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Icee8bf4978365a23d6627e7bc411b63f53540a3b
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] KUDU-1580 retry tserver RPC if negotiation times out

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/6926

to look at the new patch set (#5).

Change subject: KUDU-1580 retry tserver RPC if negotiation times out
......................................................................

KUDU-1580 retry tserver RPC if negotiation times out

This patch addresses KUDU-1580, i.e. with this patch the Kudu C++ client
retries an RPC with other tablet replica if the connection negotiation
with current replica timed out.

Added new integration test to cover the updated client's behavior.

Change-Id: Icee8bf4978365a23d6627e7bc411b63f53540a3b
---
M src/kudu/client/batcher.cc
M src/kudu/integration-tests/client_failover-itest.cc
M src/kudu/integration-tests/raft_consensus-itest.cc
M src/kudu/rpc/connection.cc
M src/kudu/rpc/outbound_call.cc
M src/kudu/rpc/outbound_call.h
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/rpc_controller.cc
M src/kudu/rpc/rpc_controller.h
M src/kudu/rpc/rpc_introspection.proto
12 files changed, 257 insertions(+), 38 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/26/6926/5
-- 
To view, visit http://gerrit.cloudera.org:8080/6926
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Icee8bf4978365a23d6627e7bc411b63f53540a3b
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] KUDU-1580 retry tserver RPC if negotiation times out

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/6926

to look at the new patch set (#6).

Change subject: KUDU-1580 retry tserver RPC if negotiation times out
......................................................................

KUDU-1580 retry tserver RPC if negotiation times out

This patch addresses KUDU-1580, i.e. with this patch the Kudu C++ client
retries an RPC with other tablet replica if the connection negotiation
with current replica timed out.

Added new integration test to cover the updated client's behavior.

Change-Id: Icee8bf4978365a23d6627e7bc411b63f53540a3b
---
M src/kudu/client/batcher.cc
M src/kudu/integration-tests/client_failover-itest.cc
M src/kudu/integration-tests/raft_consensus-itest.cc
M src/kudu/rpc/connection.cc
M src/kudu/rpc/outbound_call.cc
M src/kudu/rpc/outbound_call.h
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/rpc_controller.cc
M src/kudu/rpc/rpc_controller.h
M src/kudu/rpc/rpc_introspection.proto
12 files changed, 257 insertions(+), 38 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/26/6926/6
-- 
To view, visit http://gerrit.cloudera.org:8080/6926
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Icee8bf4978365a23d6627e7bc411b63f53540a3b
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] KUDU-1580 retry tserver RPC if negotiation times out

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.

Change subject: KUDU-1580 retry tserver RPC if negotiation times out
......................................................................


Patch Set 7:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6926/7/src/kudu/rpc/outbound_call.cc
File src/kudu/rpc/outbound_call.cc:

Line 331:     case NEGOTIATION_TIMED_OUT:       // fall-through
> Hmm it looks like it's only actually used by an internal runtime assertion.
Yep, it seems so.  As I understand it's fine with the change and also we have coverage for that by all our tests in DEBUG, ASAN and TSAN configurations.


-- 
To view, visit http://gerrit.cloudera.org:8080/6926
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Icee8bf4978365a23d6627e7bc411b63f53540a3b
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1580 retry tserver RPC if negotiation times out

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/6926

to look at the new patch set (#7).

Change subject: KUDU-1580 retry tserver RPC if negotiation times out
......................................................................

KUDU-1580 retry tserver RPC if negotiation times out

This patch addresses KUDU-1580, i.e. with this patch the Kudu C++ client
retries an RPC with other tablet replica if the connection negotiation
with current replica timed out.

Added new integration test to cover the updated client's behavior.

Change-Id: Icee8bf4978365a23d6627e7bc411b63f53540a3b
---
M src/kudu/client/batcher.cc
M src/kudu/integration-tests/client_failover-itest.cc
M src/kudu/integration-tests/raft_consensus-itest.cc
M src/kudu/rpc/connection.cc
M src/kudu/rpc/outbound_call.cc
M src/kudu/rpc/outbound_call.h
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/rpc_controller.cc
M src/kudu/rpc/rpc_controller.h
M src/kudu/rpc/rpc_introspection.proto
12 files changed, 264 insertions(+), 39 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/26/6926/7
-- 
To view, visit http://gerrit.cloudera.org:8080/6926
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Icee8bf4978365a23d6627e7bc411b63f53540a3b
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] KUDU-1580 retry tserver RPC if negotiation times out

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.

Change subject: KUDU-1580 retry tserver RPC if negotiation times out
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6926/1/src/kudu/rpc/connection.cc
File src/kudu/rpc/connection.cc:

PS1, Line 170:       const Status reported_status = status.IsTimedOut()
             :           ? Status::Incomplete("RPC is not sent due to connection error",
             :                                status.ToString())
             :           : status;
             :  
this seems like a very risky change to me, because now anywhere in the whole codebase where we were checking for IsTimedOut may now have an unexpected Incomplete status instead.

Perhaps we would be better off giving the RpcController and OutboundCall object some more getters to try to distinguish underlying specific causes for different timeouts? eg call->SetFailed(status, NEGOTIATION_TIMED_OUT, error.release())

or whatever?


-- 
To view, visit http://gerrit.cloudera.org:8080/6926
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Icee8bf4978365a23d6627e7bc411b63f53540a3b
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1580 retry tserver RPC if negotiation times out

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/6926

to look at the new patch set (#3).

Change subject: KUDU-1580 retry tserver RPC if negotiation times out
......................................................................

KUDU-1580 retry tserver RPC if negotiation times out

This patch addresses KUDU-1580, i.e. with this patch the Kudu C++ client
retries an RPC with other tablet replica if the connection negotiation
with current replica timed out.

Added new integration test to cover the updated client's behavior.

Change-Id: Icee8bf4978365a23d6627e7bc411b63f53540a3b
---
M src/kudu/client/batcher.cc
M src/kudu/integration-tests/client_failover-itest.cc
M src/kudu/integration-tests/raft_consensus-itest.cc
M src/kudu/rpc/connection.cc
M src/kudu/rpc/outbound_call.cc
M src/kudu/rpc/outbound_call.h
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/rpc_controller.cc
M src/kudu/rpc/rpc_controller.h
M src/kudu/rpc/rpc_introspection.proto
12 files changed, 251 insertions(+), 39 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/26/6926/3
-- 
To view, visit http://gerrit.cloudera.org:8080/6926
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Icee8bf4978365a23d6627e7bc411b63f53540a3b
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] KUDU-1580 retry tserver RPC if negotiation times out

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/6926

to look at the new patch set (#4).

Change subject: KUDU-1580 retry tserver RPC if negotiation times out
......................................................................

KUDU-1580 retry tserver RPC if negotiation times out

This patch addresses KUDU-1580, i.e. with this patch the Kudu C++ client
retries an RPC with other tablet replica if the connection negotiation
with current replica timed out.

Added new integration test to cover the updated client's behavior.

Change-Id: Icee8bf4978365a23d6627e7bc411b63f53540a3b
---
M src/kudu/client/batcher.cc
M src/kudu/integration-tests/client_failover-itest.cc
M src/kudu/integration-tests/raft_consensus-itest.cc
M src/kudu/rpc/connection.cc
M src/kudu/rpc/outbound_call.cc
M src/kudu/rpc/outbound_call.h
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/rpc_controller.cc
M src/kudu/rpc/rpc_controller.h
M src/kudu/rpc/rpc_introspection.proto
12 files changed, 251 insertions(+), 39 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/26/6926/4
-- 
To view, visit http://gerrit.cloudera.org:8080/6926
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Icee8bf4978365a23d6627e7bc411b63f53540a3b
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] KUDU-1580 retry tserver RPC if negotiation times out

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.

Change subject: KUDU-1580 retry tserver RPC if negotiation times out
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6926/1/src/kudu/rpc/connection.cc
File src/kudu/rpc/connection.cc:

PS1, Line 170:       const Status reported_status = status.IsTimedOut()
             :           ? Status::Incomplete("RPC is not sent due to connection error",
             :                                status.ToString())
             :           : status;
             :  
> this seems like a very risky change to me, because now anywhere in the whol
This sounds like a better idea.  Thanks!

Actually, while revving these set of patches I even started thinking about adding something like that into Status itself.  But putting something like that into RpcController and/or Outbound call sounds like a more proper place for that.


-- 
To view, visit http://gerrit.cloudera.org:8080/6926
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Icee8bf4978365a23d6627e7bc411b63f53540a3b
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes