Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/13607 )
Change subject: IMPALA-7802: Close connections of idle client sessions ...................................................................... Patch Set 2: (10 comments) http://gerrit.cloudera.org:8080/#/c/13607/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13607/1//COMMIT_MSG@30 PS1, Line 30: > I do wonder if we should try to handle the specific case where a connection IIRC, most of the problem involving load balancer we saw in the past never quite made it to the point of completing the connection establishment handshake. In particular, most of them were stuck in the stage of SASL / Kerberos step. There is already a timeout for that step although it's a bit high by default (5 mins) but that's to prevent false positive of closing client connection when KDC may be overloaded. 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: // Setting a socket timeout for process() may lead to false positive > Ok, makes sense. Maybe leave a comment here to explain that? Done http://gerrit.cloudera.org:8080/#/c/13607/1/be/src/rpc/thrift-server.h File be/src/rpc/thrift-server.h: http://gerrit.cloudera.org:8080/#/c/13607/1/be/src/rpc/thrift-server.h@181 PS1, Line 181: /// Called by the Thrift server implementation when it has acquired its resources and > line too long (92 > 90) Done http://gerrit.cloudera.org:8080/#/c/13607/1/be/src/rpc/thrift-server.h@182 PS1, Line 182: /// is ready to serve, and signals to StartAndWaitForServer that start-up is finished. > line too long (92 > 90) Done http://gerrit.cloudera.org:8080/#/c/13607/1/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/13607/1/be/src/service/impala-server.cc@221 PS1, Line 221: idle_session_timeout > Understood. It would be good to make it clear in the docs that idle_client_ Points taken. A doc bug will be filed once this patch is in. http://gerrit.cloudera.org:8080/#/c/13607/1/be/src/service/impala-server.cc@2072 PS1, Line 2072: unique_lock<mutex> l(connection_to_sessions_map_lock_); > What happens if the entry is present but the set is empty? Shouldn't that b DCHECK added. http://gerrit.cloudera.org:8080/#/c/13607/1/be/src/service/impala-server.cc@2074 PS1, Line 2074: > Why not session_ids = it->second? To avoid the double-lookup. Done http://gerrit.cloudera.org:8080/#/c/13607/1/be/src/service/impala-server.cc@2086 PS1, Line 2086: // Check if all the sessions associated with the connection are idle. > Minor thing, but this copy increments the refcount, and I don't think it's Done 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@119 PS1, Line 119: @ > flake8: E303 too many blank lines (2) Done 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): > I guess it's still expected that these will stay open, so maybe we don't ne Done -- 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: 2 Gerrit-Owner: Michael Ho <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Manish Maheshwari <[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: Mon, 17 Jun 2019 20:48:13 +0000 Gerrit-HasComments: Yes
