You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Michael Ho (Code Review)" <ge...@cloudera.org> on 2019/06/13 04:30:51 UTC
[Impala-ASF-CR] IMPALA-7802: Close connections of idle client sessions
Michael Ho has uploaded this change for review. ( http://gerrit.cloudera.org:8080/13607
Change subject: IMPALA-7802: Close connections of idle client sessions
......................................................................
IMPALA-7802: Close connections of idle client sessions
Previously, if idle session timeout is set either via
startup flag or query options, a client session will expire
after that set period of inactivity. However, the network
connection and the service thread of an expired session will
still be around until the session is closed by the client.
This is highly undesirable as these idle sessions still count
towards the quota bound by --fe_esrvice_threads, so if the
total number of sessions (including the idle ones) reaches
that upper bound, all incoming new session will block until
some of the existing sessions exit. There is no time bound on
when those expired sessions will be closed. In some sense,
leaving many idle sessions opened is a denial-of-service attack
on Impala.
This change implements support for closing expired client sessions.
In particular, a new flag --idle_client_poll_time_ms is added to
specify a time interval in milliseconds of client's inactivity which
will cause an idle service thread of a client connection to wake up
and check if all sessions associated with the connection are idle.
If so, the connection will be closed. This allows the service threads
to be freed up without waiting for client to close the connections.
Testing done:
- core build
- new targeted test which verifies the connections of expired sessions
are closed.
Change-Id: I97c4fb8e1b741add273f8a913fb0967303683e38
---
M be/src/rpc/TAcceptQueueServer.cpp
M be/src/rpc/TAcceptQueueServer.h
M be/src/rpc/thrift-server.cc
M be/src/rpc/thrift-server.h
M be/src/rpc/thrift-util.cc
M be/src/rpc/thrift-util.h
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M tests/custom_cluster/test_hs2.py
M tests/custom_cluster/test_session_expiration.py
10 files changed, 291 insertions(+), 85 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/07/13607/1
--
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: newchange
Gerrit-Change-Id: I97c4fb8e1b741add273f8a913fb0967303683e38
Gerrit-Change-Number: 13607
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
[Impala-ASF-CR] IMPALA-7802: Close connections of idle client sessions
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
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 1:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/13607/1//COMMIT_MSG
Commit Message:
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
> I was trying to understand this scenario:
If there are no open sessions associated with a connection, the connection_to_session state map shouldn't find an entry if I understand the code correctly. In that case, we will by default mark the connection as not idle. Please see https://gerrit.cloudera.org/#/c/13607/1/be/src/service/impala-server.cc@2072
Given the above, I am not sure I understand the concern about setting it to a very low 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 ha
It's the former for sure. Will update commit message to make it clearer.
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 pro
Yes but that's not the intention of this patch to address those cases. It's hard to tell between a slow network/client vs a stuck client so setting a timeout here may sometimes lead to false positive and prematurely closes a client's connection.
A better thing to do instead of a socket timeout is to use TCP keepalive or something similar to TCP_USER_TIMEOUT to check the remote end is still alive.
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
> So if idle_session_timeout is set to 10 minutes and idle_client_poll_period
To the first point, yes, that's potentially the latest time the connection of an idle session will be closed.
Nope. If idle_session_timeout is not set for a session, a session cannot become idle by definition.
Please keep in mind that idle_session_timeout can be set per session during session establishment via query option. The global flag --idle_session_timeout is mostly there to set the default value.
--
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 <kw...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward (443)
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoram Thanga <zo...@cloudera.com>
Gerrit-Comment-Date: Fri, 14 Jun 2019 04:46:08 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7802: Close connections of idle client sessions
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/13607 )
Change subject: IMPALA-7802: Close connections of idle client sessions
......................................................................
IMPALA-7802: Close connections of idle client sessions
Previously, if idle session timeout is set either via
startup flag or query options, a client session will expire
after that set period of inactivity. However, the network
connection and the service thread of an expired session will
still be around until the session is closed by the client.
This is highly undesirable as these idle sessions still count
towards the quota bound by --fe_esrvice_threads, so if the
total number of sessions (including the idle ones) reaches
that upper bound, all incoming new session will block until
some of the existing sessions exit. There is no time bound on
when those expired sessions will be closed. In some sense,
leaving many idle sessions opened is a denial-of-service attack
on Impala.
This change implements support for closing expired client sessions.
In particular, a new flag --idle_client_poll_time_s is added to
specify a time interval in seconds of client's inactivity which
will cause an idle service thread of a client connection to wake up
and check if all sessions associated with the connection are idle.
If so, the connection will be closed. This allows the service threads
to be freed up without waiting for client to close the connections.
Testing done:
- core build
- new targeted test which verifies the connections of expired sessions
are closed.
- verified the flags function as expected in a secure cluster with Kerberos + SSL
Change-Id: I97c4fb8e1b741add273f8a913fb0967303683e38
Reviewed-on: http://gerrit.cloudera.org:8080/13607
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M be/src/rpc/TAcceptQueueServer.cpp
M be/src/rpc/TAcceptQueueServer.h
M be/src/rpc/thrift-server.cc
M be/src/rpc/thrift-server.h
M be/src/rpc/thrift-util.cc
M be/src/rpc/thrift-util.h
M be/src/runtime/client-cache.h
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M tests/custom_cluster/test_hs2.py
M tests/custom_cluster/test_session_expiration.py
11 files changed, 329 insertions(+), 94 deletions(-)
Approvals:
Impala Public Jenkins: Looks good to me, approved; Verified
--
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: merged
Gerrit-Change-Id: I97c4fb8e1b741add273f8a913fb0967303683e38
Gerrit-Change-Number: 13607
Gerrit-PatchSet: 6
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Manish Maheshwari <ma...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoram Thanga <zo...@cloudera.com>
[Impala-ASF-CR] IMPALA-7802: Close connections of idle client sessions
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/3592/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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 <kw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Zoram Thanga <zo...@cloudera.com>
Gerrit-Comment-Date: Thu, 13 Jun 2019 05:16:30 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7802: Close connections of idle client sessions
Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
Anonymous Coward (443) 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:
(1 comment)
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
So if idle_session_timeout is set to 10 minutes and idle_client_poll_period_ms is 30 seconds, idle sessions will be closed at approx 10m30s
What would happen if idle_session_timeout is not set. In that case would the idle sessions be closed after 30 seconds after they are idle?
--
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 <kw...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward (443)
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoram Thanga <zo...@cloudera.com>
Gerrit-Comment-Date: Fri, 14 Jun 2019 03:20:42 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7802: Close connections of idle client sessions
Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Tauber-Marshall 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: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13607/2/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:
http://gerrit.cloudera.org:8080/#/c/13607/2/be/src/service/impala-server.cc@219
PS2, Line 219: DEFINE_int32(idle_client_poll_period_ms, 30000, "The poll period, in milliseconds, after "
Maybe make this seconds not ms? I don't think there's a reason anyone would need to set this lower than 1s, and a very small value could end up causing contention issues with the locks that are taken.
--
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 <kw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Manish Maheshwari <ma...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoram Thanga <zo...@cloudera.com>
Gerrit-Comment-Date: Tue, 18 Jun 2019 21:44:51 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7802: Close connections of idle client sessions
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
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 <kw...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward (443)
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoram Thanga <zo...@cloudera.com>
Gerrit-Comment-Date: Fri, 14 Jun 2019 18:59:33 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7802: Close connections of idle client sessions
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
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 3: Code-Review+1
Carry Tim's +1.
--
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: 3
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Manish Maheshwari <ma...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoram Thanga <zo...@cloudera.com>
Gerrit-Comment-Date: Tue, 18 Jun 2019 21:48:04 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7802: Close connections of idle client sessions
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
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:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13607/1//COMMIT_MSG
Commit Message:
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
> We expect the session to be closed within idle_client_poll_time_ms after th
I think it's unlikely that HS2 clients are deliberately leaving connections open without any associated session - the normal thing is to open the session immediately once you're connected.
I was mainly thinking about false positives, like if a connection could be opened then immediately closed before the client actually called OpenSession().
--
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 <kw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoram Thanga <zo...@cloudera.com>
Gerrit-Comment-Date: Thu, 13 Jun 2019 18:55:20 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7802: Close connections of idle client sessions
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13607 )
Change subject: IMPALA-7802: Close connections of idle client sessions
......................................................................
Patch Set 5: Verified+1
--
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: 5
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Manish Maheshwari <ma...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoram Thanga <zo...@cloudera.com>
Gerrit-Comment-Date: Fri, 21 Jun 2019 02:34:08 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7802: Close connections of idle client sessions
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
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 <kw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoram Thanga <zo...@cloudera.com>
Gerrit-Comment-Date: Thu, 13 Jun 2019 23:25:05 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7802: Close connections of idle client sessions
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has uploaded a new patch set (#4). ( http://gerrit.cloudera.org:8080/13607 )
Change subject: IMPALA-7802: Close connections of idle client sessions
......................................................................
IMPALA-7802: Close connections of idle client sessions
Previously, if idle session timeout is set either via
startup flag or query options, a client session will expire
after that set period of inactivity. However, the network
connection and the service thread of an expired session will
still be around until the session is closed by the client.
This is highly undesirable as these idle sessions still count
towards the quota bound by --fe_esrvice_threads, so if the
total number of sessions (including the idle ones) reaches
that upper bound, all incoming new session will block until
some of the existing sessions exit. There is no time bound on
when those expired sessions will be closed. In some sense,
leaving many idle sessions opened is a denial-of-service attack
on Impala.
This change implements support for closing expired client sessions.
In particular, a new flag --idle_client_poll_time_s is added to
specify a time interval in seconds of client's inactivity which
will cause an idle service thread of a client connection to wake up
and check if all sessions associated with the connection are idle.
If so, the connection will be closed. This allows the service threads
to be freed up without waiting for client to close the connections.
Testing done:
- core build
- new targeted test which verifies the connections of expired sessions
are closed.
- verified the flags function as expected in a secure cluster with Kerberos + SSL
Change-Id: I97c4fb8e1b741add273f8a913fb0967303683e38
---
M be/src/rpc/TAcceptQueueServer.cpp
M be/src/rpc/TAcceptQueueServer.h
M be/src/rpc/thrift-server.cc
M be/src/rpc/thrift-server.h
M be/src/rpc/thrift-util.cc
M be/src/rpc/thrift-util.h
M be/src/runtime/client-cache.h
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M tests/custom_cluster/test_hs2.py
M tests/custom_cluster/test_session_expiration.py
11 files changed, 329 insertions(+), 94 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/07/13607/4
--
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: newpatchset
Gerrit-Change-Id: I97c4fb8e1b741add273f8a913fb0967303683e38
Gerrit-Change-Number: 13607
Gerrit-PatchSet: 4
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Manish Maheshwari <ma...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoram Thanga <zo...@cloudera.com>
[Impala-ASF-CR] IMPALA-7802: Close connections of idle client sessions
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
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:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13607/1//COMMIT_MSG
Commit Message:
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
Do we expect the connection to be closed idle_client_poll_time_ms after the last session is closed?
What happens if a HS2 connection is opened, then the OpenSession RPC is not called?
--
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 <kw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoram Thanga <zo...@cloudera.com>
Gerrit-Comment-Date: Thu, 13 Jun 2019 17:49:26 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7802: Close connections of idle client sessions
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has uploaded a new patch set (#2). ( http://gerrit.cloudera.org:8080/13607 )
Change subject: IMPALA-7802: Close connections of idle client sessions
......................................................................
IMPALA-7802: Close connections of idle client sessions
Previously, if idle session timeout is set either via
startup flag or query options, a client session will expire
after that set period of inactivity. However, the network
connection and the service thread of an expired session will
still be around until the session is closed by the client.
This is highly undesirable as these idle sessions still count
towards the quota bound by --fe_esrvice_threads, so if the
total number of sessions (including the idle ones) reaches
that upper bound, all incoming new session will block until
some of the existing sessions exit. There is no time bound on
when those expired sessions will be closed. In some sense,
leaving many idle sessions opened is a denial-of-service attack
on Impala.
This change implements support for closing expired client sessions.
In particular, a new flag --idle_client_poll_time_ms is added to
specify a time interval in milliseconds of client's inactivity which
will cause an idle service thread of a client connection to wake up
and check if all sessions associated with the connection are idle.
If so, the connection will be closed. This allows the service threads
to be freed up without waiting for client to close the connections.
Testing done:
- core build
- new targeted test which verifies the connections of expired sessions
are closed.
Change-Id: I97c4fb8e1b741add273f8a913fb0967303683e38
---
M be/src/rpc/TAcceptQueueServer.cpp
M be/src/rpc/TAcceptQueueServer.h
M be/src/rpc/thrift-server.cc
M be/src/rpc/thrift-server.h
M be/src/rpc/thrift-util.cc
M be/src/rpc/thrift-util.h
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M tests/custom_cluster/test_hs2.py
M tests/custom_cluster/test_session_expiration.py
10 files changed, 315 insertions(+), 85 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/07/13607/2
--
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: newpatchset
Gerrit-Change-Id: I97c4fb8e1b741add273f8a913fb0967303683e38
Gerrit-Change-Number: 13607
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Manish Maheshwari <ma...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoram Thanga <zo...@cloudera.com>
[Impala-ASF-CR] IMPALA-7802: Close connections of idle client sessions
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
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 2: Code-Review+1
Not sure if anyone else wanted to take a look - I think Thomas probably knows this part of the code better. I can upgrade to a +2 if noone else is going to look at it.
--
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 <kw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Manish Maheshwari <ma...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoram Thanga <zo...@cloudera.com>
Gerrit-Comment-Date: Tue, 18 Jun 2019 16:52:20 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7802: Close connections of idle client sessions
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/3647/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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 <kw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Manish Maheshwari <ma...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoram Thanga <zo...@cloudera.com>
Gerrit-Comment-Date: Mon, 17 Jun 2019 21:24:48 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7802: Close connections of idle client sessions
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
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 4: Code-Review+2
Carry Thomas' +2
--
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: 4
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Manish Maheshwari <ma...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoram Thanga <zo...@cloudera.com>
Gerrit-Comment-Date: Thu, 20 Jun 2019 18:02:56 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7802: Close connections of idle client sessions
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13607 )
Change subject: IMPALA-7802: Close connections of idle client sessions
......................................................................
Patch Set 5: Code-Review+2
--
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: 5
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Manish Maheshwari <ma...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoram Thanga <zo...@cloudera.com>
Gerrit-Comment-Date: Thu, 20 Jun 2019 21:28:04 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7802: Close connections of idle client sessions
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
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:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13607/2/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:
http://gerrit.cloudera.org:8080/#/c/13607/2/be/src/service/impala-server.cc@219
PS2, Line 219: DEFINE_int32(idle_client_poll_period_ms, 30000, "The poll period, in milliseconds, after "
> Maybe make this seconds not ms? I don't think there's a reason anyone would
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 <kw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Manish Maheshwari <ma...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoram Thanga <zo...@cloudera.com>
Gerrit-Comment-Date: Thu, 20 Jun 2019 17:58:17 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7802: Close connections of idle client sessions
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
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 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13607/1//COMMIT_MSG
Commit Message:
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
> I think it's unlikely that HS2 clients are deliberately leaving connections
The logic will not mark a connection as idle if we cannot find any sessions with a given connection in the connection to session state map. Based on my understanding, we don't remove the session from the connection to session state map even after it's closed so we should be fine in that case.
--
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 <kw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoram Thanga <zo...@cloudera.com>
Gerrit-Comment-Date: Thu, 13 Jun 2019 19:18:21 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7802: Close connections of idle client sessions
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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:
(3 comments)
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 is
line too long (92 > 90)
http://gerrit.cloudera.org:8080/#/c/13607/1/be/src/rpc/thrift-server.h@182
PS1, Line 182: /// ready to serve, and signals to StartAndWaitForServer that start-up is finished. From
line too long (92 > 90)
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)
--
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 <kw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Zoram Thanga <zo...@cloudera.com>
Gerrit-Comment-Date: Thu, 13 Jun 2019 04:31:30 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7802: Close connections of idle client sessions
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
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 <kw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Manish Maheshwari <ma...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoram Thanga <zo...@cloudera.com>
Gerrit-Comment-Date: Mon, 17 Jun 2019 20:48:13 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7802: Close connections of idle client sessions
Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
Anonymous Coward (443) 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:
(1 comment)
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
> To the first point, yes, that's potentially the latest time the connection
Understood. It would be good to make it clear in the docs that idle_client_poll_period_ms will close session only when idle_session_timeout is set globally or per session level
--
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 <kw...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward (443)
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoram Thanga <zo...@cloudera.com>
Gerrit-Comment-Date: Fri, 14 Jun 2019 05:34:44 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7802: Close connections of idle client sessions
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
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 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13607/1//COMMIT_MSG
Commit Message:
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
> Do we expect the connection to be closed idle_client_poll_time_ms after the
We expect the session to be closed within idle_client_poll_time_ms after the last session has _expired_.
The goal of this patch is to address clients which don't close their idle sessions in time, which is the biggest complaint from users now.
May be it's worth adding some logic to also close the connection if there are no sessions associated with it for extended period of time. Based on your experience, do you expect it to be a common thing for clients to call CloseSession() without closing the connection ?
--
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 <kw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoram Thanga <zo...@cloudera.com>
Gerrit-Comment-Date: Thu, 13 Jun 2019 18:25:14 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7802: Close connections of idle client sessions
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13607 )
Change subject: IMPALA-7802: Close connections of idle client sessions
......................................................................
Patch Set 4:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/3701/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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: 4
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Manish Maheshwari <ma...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoram Thanga <zo...@cloudera.com>
Gerrit-Comment-Date: Thu, 20 Jun 2019 18:38:07 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7802: Close connections of idle client sessions
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
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:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/13607/1//COMMIT_MSG
Commit Message:
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
> If there are no open sessions associated with a connection, the connection_
Ah ok, I missed that detail when doing my initial pass over the code. The clarification helps.
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) ||
> Yes but that's not the intention of this patch to address those cases. It's
Ok, makes sense. Maybe leave a comment here to explain that?
--
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 <kw...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward (443)
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoram Thanga <zo...@cloudera.com>
Gerrit-Comment-Date: Fri, 14 Jun 2019 18:35:25 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7802: Close connections of idle client sessions
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13607 )
Change subject: IMPALA-7802: Close connections of idle client sessions
......................................................................
Patch Set 5:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/4514/ DRY_RUN=false
--
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: 5
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Manish Maheshwari <ma...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoram Thanga <zo...@cloudera.com>
Gerrit-Comment-Date: Thu, 20 Jun 2019 21:28:05 +0000
Gerrit-HasComments: No