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/04/21 06:28:32 UTC

[kudu-CR] [rpc] reopen outbound connections mode for reactor

Alexey Serbin has uploaded a new change for review.

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

Change subject: [rpc] reopen_outbound_connections mode for reactor
......................................................................

[rpc] reopen_outbound_connections mode for reactor

Introduced a test mode for the RPC ReactorThread: reopen already
existing idle outbound connection upon making another remote call
to the same server.  This is a groundwork for follow-up patches which
contain tests requiring connection negotiation to be done upon every
RPC call.

Added a couple of new ReactorThread metrics to count total number of
server- and client-side connections open during ReactorThread lifetime.

Also, did a minor clean-up on the ReactorThread code and its inline
documentation.  Added unit test to cover the newly introduced
functionality.

Change-Id: I71ada660d8c92de1813a261e221f73017c2c764a
---
M src/kudu/rpc/connection.h
M src/kudu/rpc/messenger.h
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/reactor.h
M src/kudu/rpc/rpc-test.cc
5 files changed, 101 insertions(+), 24 deletions(-)


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

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

[kudu-CR] [rpc] reopen outbound connections mode for reactor

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

Alexey Serbin has posted comments on this change.

Change subject: [rpc] reopen_outbound_connections mode for reactor
......................................................................


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6710/3//COMMIT_MSG
Commit Message:

Line 7: [rpc] reopen_outbound_connections mode for reactor
> That's the only use-case for now.
I meant it's necessary to perform token verification at every RPC.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I71ada660d8c92de1813a261e221f73017c2c764a
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] [rpc] reopen outbound connections mode for reactor

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.

Dan Burkert has posted comments on this change.

Change subject: [rpc] reopen_outbound_connections mode for reactor
......................................................................


Patch Set 3:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/6710/3//COMMIT_MSG
Commit Message:

Line 7: [rpc] reopen_outbound_connections mode for reactor
Would it be possible to get similar functionality by providing a method which clears the reactor's current connections (or just a specific connection)?  I'm more of a fan of things that reach in and mess with internal state than special modes which change behavior.  Modes tend to make the normal execution paths more complicated (as is the case here).


http://gerrit.cloudera.org:8080/#/c/6710/3/src/kudu/rpc/connection.h
File src/kudu/rpc/connection.h:

Line 114
Why this change?  I think the thread safety of this method is critical.


http://gerrit.cloudera.org:8080/#/c/6710/3/src/kudu/rpc/reactor.cc
File src/kudu/rpc/reactor.cc:

Line 359:     c->Shutdown(Status::NetworkError("connection is closed due to non-reuse policy"));
Will this cause issues if the connection has in-flight RPCs?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I71ada660d8c92de1813a261e221f73017c2c764a
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] [rpc] reopen outbound connections mode for reactor

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.

Dan Burkert has posted comments on this change.

Change subject: [rpc] reopen_outbound_connections mode for reactor
......................................................................


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6710/3//COMMIT_MSG
Commit Message:

Line 7: [rpc] reopen_outbound_connections mode for reactor
> I meant it's necessary to perform token verification at every RPC.
Add a private method to the client (or somewhere accessible) that will close all idle RPCs, and expose that to that specific test.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I71ada660d8c92de1813a261e221f73017c2c764a
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] [rpc] reopen outbound connections mode for reactor

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

Alexey Serbin has uploaded a new patch set (#2).

Change subject: [rpc] reopen_outbound_connections mode for reactor
......................................................................

[rpc] reopen_outbound_connections mode for reactor

Introduced a test mode for the RPC ReactorThread: reopen already
existing idle outbound connection upon making another remote call
to the same server.  This is a groundwork for follow-up patches which
contain tests requiring connection negotiation to be done upon every
RPC call.

Added a couple of new ReactorThread metrics to count total number of
server- and client-side connections open during ReactorThread lifetime.

Added unit test to cover the newly introduced functionality.  Also, did
a minor clean-up on the ReactorThread code and its inline documentation.

Change-Id: I71ada660d8c92de1813a261e221f73017c2c764a
---
M src/kudu/rpc/connection.h
M src/kudu/rpc/messenger.h
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/reactor.h
M src/kudu/rpc/rpc-test.cc
5 files changed, 102 insertions(+), 24 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I71ada660d8c92de1813a261e221f73017c2c764a
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] [rpc] reopen outbound connections mode for reactor

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

Alexey Serbin has posted comments on this change.

Change subject: [rpc] reopen_outbound_connections mode for reactor
......................................................................


Patch Set 3:

> I'm still concerned about different modes, but given this
 > particular one is fairly non-invasive let's go ahead with it.

Thank you for the review.  I found this approach the best what I could see so far.  We can update this and make it less invasive if some new ideas appear down the way.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I71ada660d8c92de1813a261e221f73017c2c764a
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] [rpc] reopen outbound connections mode for reactor

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.

Dan Burkert has posted comments on this change.

Change subject: [rpc] reopen_outbound_connections mode for reactor
......................................................................


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6710/3//COMMIT_MSG
Commit Message:

Line 7: [rpc] reopen_outbound_connections mode for reactor
> I don't think that would give a good coverage -- the client code contains m
There are a couple of issues I have with modes.

If you flip to a different mode only for tests, then you aren't testing the production code, you are testing that specific mode.  How can you be sure that you don't have a bug which only happens when connections are long lived?  I agree that it's difficult to inject connection resets in the middle of a retry chain, but I also think it's problematic that these tests aren't testing the messenger as it behaves in the real world.

Modes become even more problematic when there are multiple modes, all changing behavior in subtle and potentially correlated ways.

I don't think either of these concerns applies to fault injection.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I71ada660d8c92de1813a261e221f73017c2c764a
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] [rpc] reopen outbound connections mode for reactor

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

Alexey Serbin has posted comments on this change.

Change subject: [rpc] reopen_outbound_connections mode for reactor
......................................................................


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6710/3//COMMIT_MSG
Commit Message:

Line 7: [rpc] reopen_outbound_connections mode for reactor
> Add a private method to the client (or somewhere accessible) that will clos
From where do I call that private method?  How do I make sure it's being called after every RPC call?  Do you suggest to add that in many places in the client code?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I71ada660d8c92de1813a261e221f73017c2c764a
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] [rpc] reopen outbound connections mode for reactor

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

Alexey Serbin has submitted this change and it was merged.

Change subject: [rpc] reopen_outbound_connections mode for reactor
......................................................................


[rpc] reopen_outbound_connections mode for reactor

Introduced a test mode for the RPC ReactorThread: reopen already
existing idle outbound connection upon making another remote call
to the same server.  This is a groundwork for follow-up patches which
contain tests requiring connection negotiation to be done upon every
RPC call.

Added a couple of new ReactorThread metrics to count total number of
server- and client-side connections open during ReactorThread lifetime.

Added unit test to cover the newly introduced functionality.  Also, did
a minor clean-up on the ReactorThread code and its inline documentation.

Change-Id: I71ada660d8c92de1813a261e221f73017c2c764a
Reviewed-on: http://gerrit.cloudera.org:8080/6710
Tested-by: Kudu Jenkins
Reviewed-by: Dan Burkert <da...@apache.org>
---
M src/kudu/rpc/connection.h
M src/kudu/rpc/messenger.h
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/reactor.h
M src/kudu/rpc/rpc-test.cc
5 files changed, 102 insertions(+), 24 deletions(-)

Approvals:
  Dan Burkert: Looks good to me, approved
  Kudu Jenkins: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I71ada660d8c92de1813a261e221f73017c2c764a
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] [rpc] reopen outbound connections mode for reactor

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

Alexey Serbin has posted comments on this change.

Change subject: [rpc] reopen_outbound_connections mode for reactor
......................................................................


Patch Set 3:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/6710/3//COMMIT_MSG
Commit Message:

Line 7: [rpc] reopen_outbound_connections mode for reactor
> Would it be possible to get similar functionality by providing a method whi
I think it's possible, but that would require a lot more involvement from the other party which wants to benefit from the side-effects of calling the method or having this new behavior.

In other words, I'm not quite sure whether it would be possible to close every idle client->server connection after every RPC call made by a client without much surgery in many places of the C++ client library.


http://gerrit.cloudera.org:8080/#/c/6710/3/src/kudu/rpc/connection.h
File src/kudu/rpc/connection.h:

Line 114
> Why this change?  I think the thread safety of this method is critical.
Because this comment is outdated: from connection.cc you can see this method can be called from the reactor thread only:

DCHECK(reactor_thread_->IsCurrentThread());


http://gerrit.cloudera.org:8080/#/c/6710/3/src/kudu/rpc/reactor.cc
File src/kudu/rpc/reactor.cc:

Line 359:     c->Shutdown(Status::NetworkError("connection is closed due to non-reuse policy"));
> Will this cause issues if the connection has in-flight RPCs?
This would be a problem if that were not an idle connection.  The connections with in-flight RPCs are not to be tread this way.  For them, there is a special treatment under the 'if (!c->Idle())' case above.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I71ada660d8c92de1813a261e221f73017c2c764a
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] [rpc] reopen outbound connections mode for reactor

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.

Dan Burkert has posted comments on this change.

Change subject: [rpc] reopen_outbound_connections mode for reactor
......................................................................


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6710/3//COMMIT_MSG
Commit Message:

Line 7: [rpc] reopen_outbound_connections mode for reactor
> I think it's possible, but that would require a lot more involvement from t
Oh, I thought this mode was intended only for the new tests you are writing.  Are there other usecases?  I think if it's just for tests it's better for the test to be more verbose than to introduce extra runtime modes in production code.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I71ada660d8c92de1813a261e221f73017c2c764a
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] [rpc] reopen outbound connections mode for reactor

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.

Dan Burkert has posted comments on this change.

Change subject: [rpc] reopen_outbound_connections mode for reactor
......................................................................


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6710/3//COMMIT_MSG
Commit Message:

Line 7: [rpc] reopen_outbound_connections mode for reactor
> From where do I call that private method?  How do I make sure it's being ca
> From where do I call that private method?

From the test.

> How do I make sure it's being called after every RPC call?

Call it before every operation that you want to test retrieves a token correctly.

>  Do you suggest to add that in many places in the client code?

The point is not to change the production client code at all, but instead to make the test reach in and change state as necessary.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I71ada660d8c92de1813a261e221f73017c2c764a
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] [rpc] reopen outbound connections mode for reactor

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

Alexey Serbin has posted comments on this change.

Change subject: [rpc] reopen_outbound_connections mode for reactor
......................................................................

Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6710/3//COMMIT_MSG
Commit Message:

Line 7: [rpc] reopen_outbound_connections mode for reactor
> > From where do I call that private method?
I don't think that would give a good coverage -- the client code contains many places where something is done _internally_ in the client, e.g. retries. In other words, the client code contains a lot of places which has code which does compound operations -- to cover that, it would be necessary to insert those calls not only from the test code, but do so in those places as well.

I don't think it's a viable alternative.

BTW, where does that point of not changing the production code comes from? E.g. currently there are places where fault injects is done in the production code.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I71ada660d8c92de1813a261e221f73017c2c764a
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] [rpc] reopen outbound connections mode for reactor

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.

Dan Burkert has posted comments on this change.

Change subject: [rpc] reopen_outbound_connections mode for reactor
......................................................................


Patch Set 4:

I'm still concerned about different modes, but given this particular one is fairly non-invasive let's go ahead with it.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I71ada660d8c92de1813a261e221f73017c2c764a
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] [rpc] reopen outbound connections mode for reactor

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

Alexey Serbin has posted comments on this change.

Change subject: [rpc] reopen_outbound_connections mode for reactor
......................................................................


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6710/3//COMMIT_MSG
Commit Message:

Line 7: [rpc] reopen_outbound_connections mode for reactor
> Oh, I thought this mode was intended only for the new tests you are writing
That's the only use-case for now.

So, for something like https://gerrit.cloudera.org/#/c/6648/ what would 'more verbose' mean?  I need to have connection re-negotiation happen after at every call.

What would you suggest here instead?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I71ada660d8c92de1813a261e221f73017c2c764a
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] [rpc] reopen outbound connections mode for reactor

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.

Dan Burkert has posted comments on this change.

Change subject: [rpc] reopen_outbound_connections mode for reactor
......................................................................


Patch Set 4: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I71ada660d8c92de1813a261e221f73017c2c764a
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] [rpc] reopen outbound connections mode for reactor

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

Alexey Serbin has posted comments on this change.

Change subject: [rpc] reopen_outbound_connections mode for reactor
......................................................................

Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6710/3//COMMIT_MSG
Commit Message:

Line 7: [rpc] reopen_outbound_connections mode for reactor
> There are a couple of issues I have with modes.
Well, most of our tests either have run-time parameters different from what they are by default in production, or turn on some non-production modes (e.g. fault injection and alike) to exercise some particular code paths for better coverage. And the follow-up tests are not exception. We could look at this new option as the boundary case of the RPC keepalive-related parameters -- in the real production, connections are closed after some period of inactivity (65 seconds by default).

The regular long-lived connection scenarios are not in the scope of coming new tests: the authn token is verified only when connection is established, as I understand. Those long-lived scenarios cannot be addressed in the scope of our integration tests because they cannot run for too long without being killed by the test framework. If you think it's necessary to address those and specifically target some possible issues you know about, let's open a separate JIRA item for that.

Yes, combination of different modes might give a huge matrix of different behaviors to test and verify. However, adding a multitude of change-of-behavior points in the client code is not any better: it would not be like in the production code anyway, and it would change the behavior in many places, so variation matrix would be of much greater dimensions. As I see it, if not doing multiple incisions, current client code does not allow to implement a clean cut, driven-from-the-test-only scenario which would give good coverage for authn token verification paths. If not touching the production code, then it would be necessary to implement a massive mock proxy for the client. The latter wouldn't give any assurance that it would be close to the behavior of the real production code.

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