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 <k...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Comment-Date: Tue, 19 Dec 2017 08:04:00 +0000 Gerrit-HasComments: Yes
