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 2017/12/13 23:28:29 UTC

[kudu-CR] KUDU-2237: Allow idle server connection scanning to be disabled

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


Change subject: KUDU-2237: Allow idle server connection scanning to be disabled
......................................................................

KUDU-2237: Allow idle server connection scanning to be disabled

Currently, a server connection being idle for more than
FLAGS_rpc_default_keepalive_time_ms ms will be closed.
However, certain services (e.g. Impala) using KRPC may want to
keep the idle connections alive for various reasons (e.g. sheer
number of connections to re-establish,  negotiation overhead
in a secure cluster). To avoid idle connection from being
closed, one currently have to set FLAGS_rpc_default_keepalive_time_ms
to a very large value.

This change implements a cleaner solution by disabling idle
connection scanning if FLAGS_rpc_default_keepalive_time_ms is
set to 0. This avoids the unnecessary overhead of scanning
for idle server connections and alleviates the user from having
to pick a random large number to make sure the connection is
always kept alive.

Change-Id: I6161b9e753f05620784565a417d248acf8e7050a
---
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/messenger.h
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/reactor.h
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc-test.cc
6 files changed, 102 insertions(+), 28 deletions(-)



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

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

[kudu-CR] KUDU-2237: Allow idle server connection scanning to be disabled

Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.

Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/8831 )

Change subject: KUDU-2237: Allow idle server connection scanning to be disabled
......................................................................

Patch Set 6:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/8831/5//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/8831/5//COMMIT_MSG@20
PS5, Line 20: set to -1 or any negative value. This avoids the unnecessary
> I think -1 would make more sense as a special value. '0' sounds like 'keepa
Makes sense. In fact, setting it to a negative value won't be too meaningful so idle connection detection is only run when keep_alive_time_ms >= 0. Please let me know if you think otherwise.

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

http://gerrit.cloudera.org:8080/#/c/8831/5/src/kudu/rpc/rpc-test.cc@402
PS5, Line 402: // Set up server.
> agreed with tidybot here
Some earlier version without {} gets flagged by LINT:

/home/jenkins-slave/workspace/kudu-master/0/src/kudu/rpc/rpc-test.cc:400: Empty loop bodies should use {} or continue [whitespace/empty_loop_body] [5]

I can switch to using an empty body. In fact, this is gone in PS6.

http://gerrit.cloudera.org:8080/#/c/8831/5/src/kudu/rpc/rpc-test.cc@407
PS5, Line 407: nt.
> 'AlwaysKeepalive" is more accurate than 'NoTimeout' since we use the term '
Done

http://gerrit.cloudera.org:8080/#/c/8831/5/src/kudu/rpc/rpc-test.cc@435
PS5, Line 435:
: // Test that outbound connections to the same server are reopen upon every RPC
: // call when the 'rpc_reopen_outbound_connections' flag is set.
: TEST_P(TestRpc, TestReopenOutboundConnections) {
: // Set the flag to enable special mode: close and reopen already established
> I'm not entirely following this test. You're trying to trigger KUDU-279, bu
The simple test you suggested is essentially line 445 - line 463 below. What I am concerned about the simple test is that we cannot really tell if the connection is still alive because the thread didn't sleep for long enough.

I was also a bit uncertain whether to keep the more complicated test. On one hand, it serves as an example which inspired this change to begin with and verifies that setting keepalive_time_ms_ = 0 will keep the connection alive and "solves" the problem. On the other hand, this seems a bit hard to follow and it may break once KUDU-279 is fixed.

I simplify this call to a synchronous RPC in the new PS and keeps the simple test around.

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6161b9e753f05620784565a417d248acf8e7050a
Gerrit-Change-Number: 8831
Gerrit-PatchSet: 6
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 19 Dec 2017 08:04:00 +0000
Gerrit-HasComments: Yes