Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13607 )
Change subject: IMPALA-7802: Close connections of idle client sessions ...................................................................... Patch Set 1: (5 comments) I had a few more questions. Somewhat still trying to understand the scope and expected behaviour. The implementation looks sane but need to think more deeply about the correctness once I understand the expected behaviour. http://gerrit.cloudera.org:8080/#/c/13607/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13607/1//COMMIT_MSG@15 PS1, Line 15: fe_esrvice_threads typo http://gerrit.cloudera.org:8080/#/c/13607/1//COMMIT_MSG@28 PS1, Line 28: If so, the connection will be closed. This allows the service threads > The logic will not mark a connection as idle if we cannot find any sessions I was trying to understand this scenario: t=0: Client connects, then does nothing t=x: Client calls OpenSession() My current understanding is that if x + <network latency> < --idle_client_poll_time_ms, OpenSession() succeeds. But if x + <network latency> >= --idle_client_poll_time_ms, then the connection may get timed out, because the underlying poll() call will hit the timeout, return, and discover there are no open sessions. I looked at thrift's poll implementation and the poll docs and it seems to guarantee no early wakeups (aside from EINTR, which is handled by thrift). If my current understanding is correct, I think this is ok, except it could result in some really weird behaviour if it was set to a very low value, since there would be a race This might mean we need to document that it shouldn't be set to a very low value, or that we should enforce a minimum value. http://gerrit.cloudera.org:8080/#/c/13607/1//COMMIT_MSG@30 PS1, Line 30: One thing I didn't understand about the scope of this - are we trying to hand the specific case where a "normally-behaving" client connects, sends some RPCs, then goes idle, or are we also trying to handle more pathological cases where clients stop mid-way through RPCs or similar? http://gerrit.cloudera.org:8080/#/c/13607/1/be/src/rpc/TAcceptQueueServer.cpp File be/src/rpc/TAcceptQueueServer.cpp: http://gerrit.cloudera.org:8080/#/c/13607/1/be/src/rpc/TAcceptQueueServer.cpp@78 PS1, Line 78: if (!processor_->process(input_, output_, connectionContext) || Is it possible that the thread can get stuck in either the process() or processContext() call? http://gerrit.cloudera.org:8080/#/c/13607/1/tests/custom_cluster/test_session_expiration.py File tests/custom_cluster/test_session_expiration.py: http://gerrit.cloudera.org:8080/#/c/13607/1/tests/custom_cluster/test_session_expiration.py@122 PS1, Line 122: def test_closing_idle_connection(self, vector): Should we also check that sockets that *don't* create a session get closed. E.g. if a socket is opened that does nothing and just sits there. -- To view, visit http://gerrit.cloudera.org:8080/13607 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I97c4fb8e1b741add273f8a913fb0967303683e38 Gerrit-Change-Number: 13607 Gerrit-PatchSet: 1 Gerrit-Owner: Michael Ho <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Thomas Marshall <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: Zoram Thanga <[email protected]> Gerrit-Comment-Date: Thu, 13 Jun 2019 23:25:05 +0000 Gerrit-HasComments: Yes
