Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/21128 )
Change subject: IMPALA-12264: Add limit on number of HS2 sessions per user. ...................................................................... Patch Set 2: (12 comments) > Ideally we we might throttle any rogue application in > TAcceptQueueServer::SetupConnection() which processes a new connection. The > trouble is we don’t know the user until we run authentication, which is done > by the TAcceptQueueServer::Task which processes thrift messages in a new > thread. I see. Thanks for the explanation! I think this is better than nothing anyway. On the other hand, we need some doc update to mention that when coordinators are used under a LB, applications can retry the connection if got rejected by one coordinator, since on another coordinator the count might not reach the limit yet. We can do this in a separate DOC JIRA. BTW, good to see lots of typos got fixed. :) http://gerrit.cloudera.org:8080/#/c/21128/2/be/src/service/impala-hs2-server.cc File be/src/service/impala-hs2-server.cc: http://gerrit.cloudera.org:8080/#/c/21128/2/be/src/service/impala-hs2-server.cc@376 PS2, Line 376: Status nit: remove this since already declared at L359 http://gerrit.cloudera.org:8080/#/c/21128/2/be/src/service/impala-hs2-server.cc@377 PS2, Line 377: HS2_RETURN_IF_ERROR(return_val, status, SQLSTATE_GENERAL_ERROR); Should we decrement the count if we return here? http://gerrit.cloudera.org:8080/#/c/21128/2/be/src/service/impala-hs2-server.cc@388 PS2, Line 388: Status nit: remove this since already declared at L359 http://gerrit.cloudera.org:8080/#/c/21128/2/be/src/service/impala-hs2-server.cc@425 PS2, Line 425: HS2_RETURN_ERROR(return_val, "User is not authorized.", SQLSTATE_GENERAL_ERROR); Same question - should we decrease the count if we return here? Or maybe we should increase the count at the end of this method. http://gerrit.cloudera.org:8080/#/c/21128/2/be/src/service/impala-hs2-server.cc@465 PS2, Line 465: if (!loads.count(key)) { This seems a bug if the session is opened successfully. Could you add a DCHECK and a warning log? http://gerrit.cloudera.org:8080/#/c/21128/2/be/src/service/impala-hs2-server.cc@475 PS2, Line 475: // Don't allow decrement below zero. nit: also add a warning log for this? http://gerrit.cloudera.org:8080/#/c/21128/2/be/src/service/impala-hs2-server.cc@481 PS2, Line 481: string nit: const string& http://gerrit.cloudera.org:8080/#/c/21128/2/be/src/service/impala-hs2-server.cc@488 PS2, Line 488: string nit: const string& http://gerrit.cloudera.org:8080/#/c/21128/2/be/src/service/impala-http-handler.cc File be/src/service/impala-http-handler.cc: http://gerrit.cloudera.org:8080/#/c/21128/2/be/src/service/impala-http-handler.cc@796 PS2, Line 796: sort(users.Begin(), users.End(), SessionCountComparer); nit: don't need to hold the lock during sorting http://gerrit.cloudera.org:8080/#/c/21128/2/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/21128/2/be/src/service/impala-server.cc@265 PS2, Line 265: by any single connected user nit: let's add "on a coordinator" to emphasize this is a local limit. http://gerrit.cloudera.org:8080/#/c/21128/2/be/src/service/impala-server.cc@2715 PS2, Line 2715: DecrementSessionCount(session_state->connected_user); Should we move this into the above scope protected by session_state->lock ? We also decrease the count even if the session is not added into 'sessions_to_remove'. It seems possible that we over decrease the count if a session first go into the else-branch at L2692, and then go into the if-branch at L2674 in the next iteration. http://gerrit.cloudera.org:8080/#/c/21128/2/tests/custom_cluster/test_session_expiration.py File tests/custom_cluster/test_session_expiration.py: http://gerrit.cloudera.org:8080/#/c/21128/2/tests/custom_cluster/test_session_expiration.py@197 PS2, Line 197: assert res['users'][0]['session_count'] == 2 Can we also verify the count finally come down to 0, e.g. by adding a sleep of 10s and close all these clients? -- To view, visit http://gerrit.cloudera.org:8080/21128 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Idd28edc352102d89774f6ece5376e7c79ae41aa8 Gerrit-Change-Number: 21128 Gerrit-PatchSet: 2 Gerrit-Owner: Andrew Sherman <[email protected]> Gerrit-Reviewer: Abhishek Rawat <[email protected]> Gerrit-Reviewer: Andrew Sherman <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Xiang Yang <[email protected]> Gerrit-Comment-Date: Tue, 19 Mar 2024 08:32:18 +0000 Gerrit-HasComments: Yes
