Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/10060 )
Change subject: [Preview]IMPALA-5216: Make admission control queuing async ...................................................................... Patch Set 4: (7 comments) Did a pass over this patchset. Will do another once you've pushed out the next iteration. http://gerrit.cloudera.org:8080/#/c/10060/2/be/src/scheduling/admission-controller.h File be/src/scheduling/admission-controller.h: http://gerrit.cloudera.org:8080/#/c/10060/2/be/src/scheduling/admission-controller.h@395 PS2, Line 395: > Done Did you miss updating this one? http://gerrit.cloudera.org:8080/#/c/10060/2/be/src/scheduling/admission-controller.cc File be/src/scheduling/admission-controller.cc: http://gerrit.cloudera.org:8080/#/c/10060/2/be/src/scheduling/admission-controller.cc@576 PS2, Line 576: queue_wait_timeout_ms = pool_cfg.queue_timeout_ms; > that makes sense, but in this case, for every admission decision, there is Yeah the context is important. http://gerrit.cloudera.org:8080/#/c/10060/4/be/src/scheduling/admission-controller.cc File be/src/scheduling/admission-controller.cc: http://gerrit.cloudera.org:8080/#/c/10060/4/be/src/scheduling/admission-controller.cc@599 PS4, Line 599: timed_out Inline this expression into the if() below so that the symmetry with the else if() branch is more obvious. http://gerrit.cloudera.org:8080/#/c/10060/4/be/src/service/impala-http-handler.cc File be/src/service/impala-http-handler.cc: http://gerrit.cloudera.org:8080/#/c/10060/4/be/src/service/impala-http-handler.cc@769 PS4, Line 769: NULL nullptr http://gerrit.cloudera.org:8080/#/c/10060/4/be/src/service/impala-http-handler.cc@778 PS4, Line 778: NULL nullptr http://gerrit.cloudera.org:8080/#/c/10060/4/tests/custom_cluster/test_admission_controller.py File tests/custom_cluster/test_admission_controller.py: http://gerrit.cloudera.org:8080/#/c/10060/4/tests/custom_cluster/test_admission_controller.py@480 PS4, Line 480: except (Exception) as e: parens around Exception not needed? http://gerrit.cloudera.org:8080/#/c/10060/4/tests/custom_cluster/test_admission_controller.py@493 PS4, Line 493: class TestAdmissionControllerStress(TestAdmissionControllerBase): I'm seeing some of these tests fail when run locally on my system - not sure if it's my issue or if the change somehow broke these tests. -- To view, visit http://gerrit.cloudera.org:8080/10060 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I989cf5b259afb8f5bc5c35590c94961c81ce88bf Gerrit-Change-Number: 10060 Gerrit-PatchSet: 4 Gerrit-Owner: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Wed, 25 Apr 2018 21:17:57 +0000 Gerrit-HasComments: Yes
