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) Took another quick look in light of the clarifications 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: > It's the former for sure. Will update commit message to make it clearer. I do wonder if we should try to handle the specific case where a connection is established and nothing is ever sent. I know we've seen some clients do things like this before, e.g. when load balancers are involved. But maybe I'm misremembering. I'm only thinking this because it seems like it might be straightforward. But yeah, I guess this opens a potential can of worms since it isn't just an extension of the old session timeout logic. 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@2072 PS1, Line 2072: if (it == connection_to_sessions_map_.end()) return false; What happens if the entry is present but the set is empty? Shouldn't that behave the same as the entry not being present? If it's an invariant that we don't have empty sets in the map, we should add a DCHECK. Maybe l2020 also should be changed, although I think it doesn't make a behavioural difference tehre. http://gerrit.cloudera.org:8080/#/c/13607/1/be/src/service/impala-server.cc@2074 PS1, Line 2074: session_ids = connection_to_sessions_map_[connection_id]; Why not session_ids = it->second? To avoid the double-lookup. http://gerrit.cloudera.org:8080/#/c/13607/1/be/src/service/impala-server.cc@2086 PS1, Line 2086: std::shared_ptr<SessionState> session_state = map_entry->second; Minor thing, but this copy increments the refcount, and I don't think it's necessary, since we're holding the session_state_map_lock_ the whole time. OK to ignore. 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. I guess it's still expected that these will stay open, so maybe we don't need to do this (or we could test explicitly that they don't get close). -- 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 <k...@cloudera.com> Gerrit-Reviewer: Anonymous Coward (443) Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Thomas Marshall <tmarsh...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Zoram Thanga <zo...@cloudera.com> Gerrit-Comment-Date: Fri, 14 Jun 2019 18:59:33 +0000 Gerrit-HasComments: Yes
