Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/8323 )
Change subject: IMPALA-1575: part 2: yield admission control resources ...................................................................... Patch Set 5: (3 comments) http://gerrit.cloudera.org:8080/#/c/8323/5/be/src/runtime/coordinator.h File be/src/runtime/coordinator.h: http://gerrit.cloudera.org:8080/#/c/8323/5/be/src/runtime/coordinator.h@454 PS5, Line 454: probably released > probably have been released Done http://gerrit.cloudera.org:8080/#/c/8323/5/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: http://gerrit.cloudera.org:8080/#/c/8323/5/be/src/runtime/coordinator.cc@831 PS5, Line 831: > nit: here and elsewhere, we seem to have gratuitous blank lines which makes Done http://gerrit.cloudera.org:8080/#/c/8323/5/tests/custom_cluster/test_admission_controller.py File tests/custom_cluster/test_admission_controller.py: http://gerrit.cloudera.org:8080/#/c/8323/5/tests/custom_cluster/test_admission_controller.py@91 PS5, Line 91: ] > there is also session close and session timeout. but those should both resu Yeah I was checking the coverage bottom-up to make sure that all of the code paths in ClientRequestState/Coordinator were covered. Those two additional code paths go through UnregisterQuery(), same as the Close() path (and a bunch of other things). It's not a bad idea to cover the other cases, but I think that would require switching the test to HS2, since beeswax doesn't have the same idea of sessions. I guess we could drop the beeswax connection and check the state of the query via the debug server or similar, but then it would probably just make more sense to switch to HS2. I can have a crack at that but no idea how easy or hard that would be. -- To view, visit http://gerrit.cloudera.org:8080/8323 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I80279eb2bda740d7f61420f52db3bfa42a6a51ac Gerrit-Change-Number: 8323 Gerrit-PatchSet: 5 Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: anujphadke <[email protected]> Gerrit-Comment-Date: Mon, 06 Nov 2017 23:26:17 +0000 Gerrit-HasComments: Yes
