You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Michael Ho (Code Review)" <ge...@cloudera.org> on 2018/10/14 00:06:45 UTC

[kudu-CR] Add "service name" as part of ConnectionId 

Michael Ho has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11681


Change subject: Add "service_name" as part of ConnectionId
......................................................................

Add "service_name" as part of ConnectionId

The motivation for doing so is to allow a client to transparently use
separate connections for different services. For instance, a server may
host multiple KRPC based services: one for control command and one for
data transfer. Separating the connections between the control channel
and the data channel prevents unnecessary delays of the control commands
due to being stuck behind large data transfers from client to server.

Change-Id: I6767e631fd9530ea54f5ed63ff4c8c179ab216b2
---
M src/kudu/rpc/connection_id.cc
M src/kudu/rpc/connection_id.h
M src/kudu/rpc/proxy.cc
3 files changed, 16 insertions(+), 5 deletions(-)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I6767e631fd9530ea54f5ed63ff4c8c179ab216b2
Gerrit-Change-Number: 11681
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Ho <kw...@cloudera.com>

[kudu-CR] Add "service name" as part of ConnectionId

Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/11681 )

Change subject: Add "service_name" as part of ConnectionId
......................................................................


Patch Set 2:

(3 comments)

Thanks for the review.

http://gerrit.cloudera.org:8080/#/c/11681/1/src/kudu/rpc/connection_id.h
File src/kudu/rpc/connection_id.h:

http://gerrit.cloudera.org:8080/#/c/11681/1/src/kudu/rpc/connection_id.h@56
PS1, Line 56:   // Returns a string representation of the object, not including the password field.
            :   std::string ToString() const;
> Looks like this isn't implemented. We can consider removing this.
Done


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

http://gerrit.cloudera.org:8080/#/c/11681/1/src/kudu/rpc/connection_id.cc@52
PS1, Line 52: string ConnectionId::ToString() const {
> Need to incorporate service_name_ here too.
Done


http://gerrit.cloudera.org:8080/#/c/11681/1/src/kudu/rpc/connection_id.cc@69
PS1, Line 69:   boost::hash_combine(seed, hostname_);
> We need to include the service_name_ as part of the HashCode too.
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6767e631fd9530ea54f5ed63ff4c8c179ab216b2
Gerrit-Change-Number: 11681
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 16 Oct 2018 00:34:20 +0000
Gerrit-HasComments: Yes

[kudu-CR] Add "service name" as part of ConnectionId

Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/11681 )

Change subject: Add "service_name" as part of ConnectionId
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11681/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/11681/1//COMMIT_MSG@7
PS1, Line 7: Add "service_name" as part of ConnectionId
> I think this needs to be a separately-configured option. In Kudu we use lot
Thanks for the idea. Will look into it.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6767e631fd9530ea54f5ed63ff4c8c179ab216b2
Gerrit-Change-Number: 11681
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 17 Oct 2018 00:43:18 +0000
Gerrit-HasComments: Yes

[kudu-CR] Add "network plane" as part of ConnectionId

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11681 )

Change subject: Add "network_plane" as part of ConnectionId
......................................................................


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11681/4/src/kudu/rpc/rpc-test.cc
File src/kudu/rpc/rpc-test.cc:

http://gerrit.cloudera.org:8080/#/c/11681/4/src/kudu/rpc/rpc-test.cc@696
PS4, Line 696: 
> Just copied and pasted that pattern from TestCredentialsPolicy. I don't see
Was cleaning my inbox and found this thread.

Just FYI: the decision to use mixed ASSERT and EXPECT in TestCredentialsPolicy is to spot more details if would a failure happen.  Without asserting on OK return status of a function that outputs some metrics it does not make sense to look at those, however, I think it's beneficial to see all mismatches, not just the first one.  That's about collecting more information on the whole picture.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6767e631fd9530ea54f5ed63ff4c8c179ab216b2
Gerrit-Change-Number: 11681
Gerrit-PatchSet: 4
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 09 Nov 2018 01:05:37 +0000
Gerrit-HasComments: Yes

[kudu-CR] Add "service name" as part of ConnectionId

Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/11681 )

Change subject: Add "service_name" as part of ConnectionId
......................................................................


Patch Set 1:

(3 comments)

Thanks for doing this. Just a few comments.

http://gerrit.cloudera.org:8080/#/c/11681/1/src/kudu/rpc/connection_id.h
File src/kudu/rpc/connection_id.h:

http://gerrit.cloudera.org:8080/#/c/11681/1/src/kudu/rpc/connection_id.h@56
PS1, Line 56:   // Copy state from another object to this one.
            :   void CopyFrom(const ConnectionId& other);
Looks like this isn't implemented. We can consider removing this.


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

http://gerrit.cloudera.org:8080/#/c/11681/1/src/kudu/rpc/connection_id.cc@52
PS1, Line 52: string ConnectionId::ToString() const {
Need to incorporate service_name_ here too.


http://gerrit.cloudera.org:8080/#/c/11681/1/src/kudu/rpc/connection_id.cc@69
PS1, Line 69:   boost::hash_combine(seed, user_credentials_.HashCode());
We need to include the service_name_ as part of the HashCode too.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6767e631fd9530ea54f5ed63ff4c8c179ab216b2
Gerrit-Change-Number: 11681
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 15 Oct 2018 23:56:59 +0000
Gerrit-HasComments: Yes

[kudu-CR] Add "network plane" as part of ConnectionId

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11681 )

Change subject: Add "network_plane" as part of ConnectionId
......................................................................

Add "network_plane" as part of ConnectionId

The motivation for doing so is to allow N services on the same host to
be multiplexed on M different connections. For instance, a server may
host multiple KRPC based services: one for control command and one for
data transfer. Separating the connections between the control channel
and the data channel prevents unnecessary delays of the control commands
due to being stuck behind large data transfers from client to server.

By default, the network_plane of a new ConnectionId is not set.
A user can change it to a different value by calling
Proxy::set_network_plane() on the ConnectionId.

Change-Id: I6767e631fd9530ea54f5ed63ff4c8c179ab216b2
Reviewed-on: http://gerrit.cloudera.org:8080/11681
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Kudu Jenkins
---
M src/kudu/rpc/connection_id.cc
M src/kudu/rpc/connection_id.h
M src/kudu/rpc/messenger.h
M src/kudu/rpc/proxy.cc
M src/kudu/rpc/proxy.h
M src/kudu/rpc/reactor.h
M src/kudu/rpc/rpc-test.cc
7 files changed, 138 insertions(+), 31 deletions(-)

Approvals:
  Adar Dembo: Looks good to me, approved
  Kudu Jenkins: Verified

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I6767e631fd9530ea54f5ed63ff4c8c179ab216b2
Gerrit-Change-Number: 11681
Gerrit-PatchSet: 6
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] Add "network plane" as part of ConnectionId

Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Sailesh Mukil, Todd Lipcon, 

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

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

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

Change subject: Add "network_plane" as part of ConnectionId
......................................................................

Add "network_plane" as part of ConnectionId

The motivation for doing so is to allow N services on the same host to
be multiplexed on M different connections. For instance, a server may
host multiple KRPC based services: one for control command and one for
data transfer. Separating the connections between the control channel
and the data channel prevents unnecessary delays of the control commands
due to being stuck behind large data transfers from client to server.

By default, the network_plane of a new ConnectionId is set to
"default_network_plane". A user can change it to a non-default
value by calling Proxy::set_network_plane() on the ConnectionId.

Change-Id: I6767e631fd9530ea54f5ed63ff4c8c179ab216b2
---
M src/kudu/rpc/connection_id.cc
M src/kudu/rpc/connection_id.h
M src/kudu/rpc/messenger.h
M src/kudu/rpc/proxy.cc
M src/kudu/rpc/proxy.h
M src/kudu/rpc/rpc-test.cc
6 files changed, 102 insertions(+), 6 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6767e631fd9530ea54f5ed63ff4c8c179ab216b2
Gerrit-Change-Number: 11681
Gerrit-PatchSet: 3
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] Add "network plane" as part of ConnectionId

Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Sailesh Mukil, Todd Lipcon, 

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

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

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

Change subject: Add "network_plane" as part of ConnectionId
......................................................................

Add "network_plane" as part of ConnectionId

The motivation for doing so is to allow N services on the same host to
be multiplexed on M different connections. For instance, a server may
host multiple KRPC based services: one for control command and one for
data transfer. Separating the connections between the control channel
and the data channel prevents unnecessary delays of the control commands
due to being stuck behind large data transfers from client to server.

By default, the network_plane of a new ConnectionId is set to
"default_network_plane". A user can change it to a different
value by calling Proxy::set_network_plane() on the ConnectionId.

Change-Id: I6767e631fd9530ea54f5ed63ff4c8c179ab216b2
---
M src/kudu/rpc/connection_id.cc
M src/kudu/rpc/connection_id.h
M src/kudu/rpc/messenger.h
M src/kudu/rpc/proxy.cc
M src/kudu/rpc/proxy.h
M src/kudu/rpc/rpc-test.cc
6 files changed, 106 insertions(+), 6 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6767e631fd9530ea54f5ed63ff4c8c179ab216b2
Gerrit-Change-Number: 11681
Gerrit-PatchSet: 4
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] Add "service name" as part of ConnectionId

Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Sailesh Mukil, Todd Lipcon, 

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

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

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

Change subject: Add "service_name" as part of ConnectionId
......................................................................

Add "service_name" as part of ConnectionId

The motivation for doing so is to allow a client to transparently use
separate connections for different services. For instance, a server may
host multiple KRPC based services: one for control command and one for
data transfer. Separating the connections between the control channel
and the data channel prevents unnecessary delays of the control commands
due to being stuck behind large data transfers from client to server.

Change-Id: I6767e631fd9530ea54f5ed63ff4c8c179ab216b2
---
M src/kudu/rpc/connection_id.cc
M src/kudu/rpc/connection_id.h
M src/kudu/rpc/proxy.cc
3 files changed, 19 insertions(+), 9 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6767e631fd9530ea54f5ed63ff4c8c179ab216b2
Gerrit-Change-Number: 11681
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] Add "network plane" as part of ConnectionId

Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/11681 )

Change subject: Add "network_plane" as part of ConnectionId
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11681/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/11681/1//COMMIT_MSG@7
PS1, Line 7: Add "service_name" as part of ConnectionId
> Thanks for the idea. Will look into it.
Sorry for the delay. I adopted option 1. Thanks.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6767e631fd9530ea54f5ed63ff4c8c179ab216b2
Gerrit-Change-Number: 11681
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 06 Nov 2018 02:13:36 +0000
Gerrit-HasComments: Yes

[kudu-CR] Add "network plane" as part of ConnectionId

Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/11681 )

Change subject: Add "network_plane" as part of ConnectionId
......................................................................


Patch Set 1:

(9 comments)

http://gerrit.cloudera.org:8080/#/c/11681/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/11681/1//COMMIT_MSG@7
PS1, Line 7: Add "service_name" as part of ConnectionId
> I'm curious to hear your thinking on, when implementing option 1, you did i
Specifying it per service seems a bit rigid as it hardcodes the data plane for all calls in the entire service.

Specifying it per-RPC is vs per-Proxy can be functionally equivalent. After all, a proxy is needed to make a RPC call. One can have multiple proxies with different data planes and pick the right one for a given call. It's also more convenient as the ConnectionId is owned by the Proxy (as the code stands now). Piping the "network plane" through per-RPC seems slightly more complicated.


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

http://gerrit.cloudera.org:8080/#/c/11681/4/src/kudu/rpc/connection_id.cc@50
PS4, Line 50: }
> Nit: we're using std::string here; don't need std:: prefix.
Done


http://gerrit.cloudera.org:8080/#/c/11681/4/src/kudu/rpc/connection_id.cc@62
PS4, Line 62:                              user_credentials_.ToString());
> Maybe omit the plane altogether if it's the default one?
Done


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

http://gerrit.cloudera.org:8080/#/c/11681/4/src/kudu/rpc/proxy.h@54
PS4, Line 54: r after a
> Nit: "so that a higher"
Done


http://gerrit.cloudera.org:8080/#/c/11681/4/src/kudu/rpc/proxy.h@55
PS4, Line 55: arted will caus
> Nit: "a control channel"
Done


http://gerrit.cloudera.org:8080/#/c/11681/4/src/kudu/rpc/proxy.h@118
PS4, Line 118:   mutable Atomic32 is_started_;
> Could we pass by copy here and std::move into the underlying ConnectionId?
Done; Did the same for set_user_credentials().


http://gerrit.cloudera.org:8080/#/c/11681/4/src/kudu/rpc/rpc-test.cc
File src/kudu/rpc/rpc-test.cc:

http://gerrit.cloudera.org:8080/#/c/11681/4/src/kudu/rpc/rpc-test.cc@696
PS4, Line 696:                                  req, &resp, &controller));
> There's a mix of ASSERT and EXPECT below; curious why you aren't just using
Just copied and pasted that pattern from TestCredentialsPolicy. I don't see any reason for such mixed use. Converted everything to ASSERT instead.


http://gerrit.cloudera.org:8080/#/c/11681/4/src/kudu/rpc/rpc-test.cc@697
PS4, Line 697: }
> Test num_{client,server}_connections here too?
Done


http://gerrit.cloudera.org:8080/#/c/11681/4/src/kudu/rpc/rpc-test.cc@698
PS4, Line 698: 
             : // Test that the RpcSidecar transfers the expected messages.
             : TEST_P(TestRpc, TestRpcSidecar) {
             :   // Set up server.
             :   Sockaddr server_addr;
             :   bool enable_ssl = GetParam();
             :   ASSERT_OK(StartTestServer(&server_addr, enable_s
> This is repeated below; perhaps define a 2-arg lambda to encapsulate the wh
These verification logic seem to be repeated across multiple tests at various places. I agree that we can encapsulate these verification logic into a function or lambda. However, the downside seems to be that it may be harder to identify the failures. As the code stands now, the failure can be easily identified by line number.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6767e631fd9530ea54f5ed63ff4c8c179ab216b2
Gerrit-Change-Number: 11681
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 08 Nov 2018 00:22:53 +0000
Gerrit-HasComments: Yes

[kudu-CR] Add "network plane" as part of ConnectionId

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/11681 )

Change subject: Add "network_plane" as part of ConnectionId
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11681/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/11681/1//COMMIT_MSG@7
PS1, Line 7: Add "service_name" as part of ConnectionId
> OK, thanks for the explanation. I think Todd's justification for per-RPC mi
yea, we could always make two proxies if we needed to. Also this isn't a public API so our decision isn't set in stone :)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6767e631fd9530ea54f5ed63ff4c8c179ab216b2
Gerrit-Change-Number: 11681
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 08 Nov 2018 19:52:15 +0000
Gerrit-HasComments: Yes

[kudu-CR] Add "service name" as part of ConnectionId

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/11681 )

Change subject: Add "service_name" as part of ConnectionId
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11681/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/11681/1//COMMIT_MSG@7
PS1, Line 7: Add "service_name" as part of ConnectionId
I think this needs to be a separately-configured option. In Kudu we use lots of separate services and putting them each on their own connection would increase the number of TCP connections pretty substantially (and without reason in many cases)

I'd lean towards one of:
1) add a new option on Proxy or RPC controller like '->set_service_plane("foo")' or even 'set_service_plane(kMyIntegerConstant)'
... which passes through to the ConnectionId.


Doing it on the Proxy is probably more convenient than doing it per-RPC call, but doing it per-RPC is a bit more flexible as even within a single service we might have some calls which are considered "control messages" (high-priority low data) and others considered data messages. (we have this case in Kudu's Consensus Service where liveness heartbeats can get stuck behind heavy data-bearing requests)

2) add an option to the RPC service itself (annotated via a protobuf service option, similar to the 'default_authz_method' option). The option would set the service plane for the service for all calls.

These options retain existing behavior for Kudu but give callers the ability to customize things, and map N services to M connections (where M <= N). In Kudu we'd probably map all of our connections that carry data onto one plane and all connections that carry only control info onto another.


Note that I'm using the term "service plane" or "network plane" or somesuch here distinct from service name itself to capture the N-M mapping.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6767e631fd9530ea54f5ed63ff4c8c179ab216b2
Gerrit-Change-Number: 11681
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 16 Oct 2018 00:43:02 +0000
Gerrit-HasComments: Yes

[kudu-CR] Add "network plane" as part of ConnectionId

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11681 )

Change subject: Add "network_plane" as part of ConnectionId
......................................................................


Patch Set 5: Code-Review+2

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11681/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/11681/1//COMMIT_MSG@7
PS1, Line 7: Add "network_plane" as part of ConnectionId
> Specifying it per service seems a bit rigid as it hardcodes the data plane 
OK, thanks for the explanation. I think Todd's justification for per-RPC might hinge on the fact that, within Kudu, we generally use one proxy for every remote+service pair. So if the service plane is a property of the proxy, we'd need to establish multiple (probably just two) proxies for each remote+service pair.

Anyway I don't have a strong feeling about this, and judging by Todd's earlier suggestions, neither does he.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6767e631fd9530ea54f5ed63ff4c8c179ab216b2
Gerrit-Change-Number: 11681
Gerrit-PatchSet: 5
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 08 Nov 2018 00:33:46 +0000
Gerrit-HasComments: Yes

[kudu-CR] Add "network plane" as part of ConnectionId

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11681 )

Change subject: Add "network_plane" as part of ConnectionId
......................................................................


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11681/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/11681/1//COMMIT_MSG@7
PS1, Line 7: Add "network_plane" as part of ConnectionId
> Sorry for the delay. I adopted option 1. Thanks.
I'm curious to hear your thinking on, when implementing option 1, you did it per-Proxy instead of per-RPC as Todd had suggested?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6767e631fd9530ea54f5ed63ff4c8c179ab216b2
Gerrit-Change-Number: 11681
Gerrit-PatchSet: 4
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 07 Nov 2018 00:08:46 +0000
Gerrit-HasComments: Yes

[kudu-CR] Add "network plane" as part of ConnectionId

Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo, Sailesh Mukil, Todd Lipcon, 

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

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

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

Change subject: Add "network_plane" as part of ConnectionId
......................................................................

Add "network_plane" as part of ConnectionId

The motivation for doing so is to allow N services on the same host to
be multiplexed on M different connections. For instance, a server may
host multiple KRPC based services: one for control command and one for
data transfer. Separating the connections between the control channel
and the data channel prevents unnecessary delays of the control commands
due to being stuck behind large data transfers from client to server.

By default, the network_plane of a new ConnectionId is not set.
A user can change it to a different value by calling
Proxy::set_network_plane() on the ConnectionId.

Change-Id: I6767e631fd9530ea54f5ed63ff4c8c179ab216b2
---
M src/kudu/rpc/connection_id.cc
M src/kudu/rpc/connection_id.h
M src/kudu/rpc/messenger.h
M src/kudu/rpc/proxy.cc
M src/kudu/rpc/proxy.h
M src/kudu/rpc/reactor.h
M src/kudu/rpc/rpc-test.cc
7 files changed, 138 insertions(+), 31 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6767e631fd9530ea54f5ed63ff4c8c179ab216b2
Gerrit-Change-Number: 11681
Gerrit-PatchSet: 5
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] Add "network plane" as part of ConnectionId

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11681 )

Change subject: Add "network_plane" as part of ConnectionId
......................................................................


Patch Set 4:

(8 comments)

Could you also update this comment in reactor.h:

  // Client-side connection map. Multiple connections could be open to a remote
  // server if multiple credential policies are used for individual RPCs.

While true, the wording should be made more generic since a connection ID is now more than just a remote and set of credentials.

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

http://gerrit.cloudera.org:8080/#/c/11681/4/src/kudu/rpc/connection_id.cc@50
PS4, Line 50: void ConnectionId::set_network_plane(std::string network_plane) {
Nit: we're using std::string here; don't need std:: prefix.


http://gerrit.cloudera.org:8080/#/c/11681/4/src/kudu/rpc/connection_id.cc@62
PS4, Line 62:   return strings::Substitute("{remote=$0, user_credentials=$1, network_plane=$2}",
Maybe omit the plane altogether if it's the default one?


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

http://gerrit.cloudera.org:8080/#/c/11681/4/src/kudu/rpc/proxy.h@54
PS4, Line 54: so higher
Nit: "so that a higher"


http://gerrit.cloudera.org:8080/#/c/11681/4/src/kudu/rpc/proxy.h@55
PS4, Line 55: control channel
Nit: "a control channel"


http://gerrit.cloudera.org:8080/#/c/11681/4/src/kudu/rpc/proxy.h@118
PS4, Line 118:   void set_network_plane(const std::string& network_plane);
Could we pass by copy here and std::move into the underlying ConnectionId?


http://gerrit.cloudera.org:8080/#/c/11681/4/src/kudu/rpc/rpc-test.cc
File src/kudu/rpc/rpc-test.cc:

http://gerrit.cloudera.org:8080/#/c/11681/4/src/kudu/rpc/rpc-test.cc@696
PS4, Line 696: 
There's a mix of ASSERT and EXPECT below; curious why you aren't just using one and not the other?


http://gerrit.cloudera.org:8080/#/c/11681/4/src/kudu/rpc/rpc-test.cc@697
PS4, Line 697:   // Verify the initial counters.
Test num_{client,server}_connections here too?


http://gerrit.cloudera.org:8080/#/c/11681/4/src/kudu/rpc/rpc-test.cc@698
PS4, Line 698:   ReactorMetrics metrics;
             :   ASSERT_OK(server_messenger_->reactors_[0]->GetMetrics(&metrics));
             :   ASSERT_EQ(0, metrics.total_client_connections_);
             :   ASSERT_EQ(0, metrics.total_server_connections_);
             :   ASSERT_OK(client_messenger->reactors_[0]->GetMetrics(&metrics));
             :   ASSERT_EQ(0, metrics.total_client_connections_);
             :   ASSERT_EQ(0, metrics.total_server_connections_);
This is repeated below; perhaps define a 2-arg lambda to encapsulate the whole thing?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6767e631fd9530ea54f5ed63ff4c8c179ab216b2
Gerrit-Change-Number: 11681
Gerrit-PatchSet: 4
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 07 Nov 2018 00:06:18 +0000
Gerrit-HasComments: Yes