Andrew Sherman 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 3: (12 comments) Thanks for the thoughtful review 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: key = > nit: remove this since already declared at L359 Ignoring as I moved the earlier declaration. http://gerrit.cloudera.org:8080/#/c/21128/2/be/src/service/impala-hs2-server.cc@377 PS2, Line 377: } > Should we decrement the count if we return here? Good question thanks. I have moved where I do the check. I think the undocumented structure of the method is that various checks are performed, and then later information about the session is persisted. I moved my new check of session counts to be at the end of the section where checks are done. http://gerrit.cloudera.org:8080/#/c/21128/2/be/src/service/impala-hs2-server.cc@388 PS2, Line 388: > nit: remove this since already declared at L359 Ignoring as I moved the earlier declaration. http://gerrit.cloudera.org:8080/#/c/21128/2/be/src/service/impala-hs2-server.cc@425 PS2, Line 425: > Same question - should we decrease the count if we return here? Or maybe we Good question again. See earlier for how I am dealing with this http://gerrit.cloudera.org:8080/#/c/21128/2/be/src/service/impala-hs2-server.cc@465 PS2, Line 465: DCHECK(false) << msg; > This seems a bug if the session is opened successfully. Could you add a DCH Done 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? Done http://gerrit.cloudera.org:8080/#/c/21128/2/be/src/service/impala-hs2-server.cc@481 PS2, Line 481: > nit: const string& Done http://gerrit.cloudera.org:8080/#/c/21128/2/be/src/service/impala-hs2-server.cc@488 PS2, Line 488: > nit: const string& Done 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 Yes, thanks 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. Done http://gerrit.cloudera.org:8080/#/c/21128/2/be/src/service/impala-server.cc@2715 PS2, Line 2715: } > Should we move this into the above scope protected by session_state->lock ? Thanks, good catch. When I wrote this I meant to put my Decrement calls near where IMPALA_SERVER_NUM_OPEN_HS2_SESSIONS is decremented, not sure why I didn't do that but I am happier now. 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 Thanks, done -- 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: 3 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: Wed, 20 Mar 2024 23:58:36 +0000 Gerrit-HasComments: Yes
