Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14104 )
Change subject: IMPALA-8803: Coordinator should release admitted memory per-backend ...................................................................... Patch Set 1: (7 comments) Did an initial pass over the review. Overall looks good. http://gerrit.cloudera.org:8080/#/c/14104/1/be/src/runtime/coordinator-backend-state.h File be/src/runtime/coordinator-backend-state.h: http://gerrit.cloudera.org:8080/#/c/14104/1/be/src/runtime/coordinator-backend-state.h@527 PS1, Line 527: const std::vector<BackendState*>& backend_states_; What owns the object that is being referenced? The schedule? http://gerrit.cloudera.org:8080/#/c/14104/1/be/src/runtime/coordinator-backend-state.cc File be/src/runtime/coordinator-backend-state.cc: http://gerrit.cloudera.org:8080/#/c/14104/1/be/src/runtime/coordinator-backend-state.cc@810 PS1, Line 810: NumReleasedBackends I feel like the name is a little confusing in isolation, I'm not sure people will have an idea what it is. Maybe NumCompletedBackends or NumResourceReleasedBackends. http://gerrit.cloudera.org:8080/#/c/14104/1/be/src/scheduling/admission-controller.h File be/src/scheduling/admission-controller.h: http://gerrit.cloudera.org:8080/#/c/14104/1/be/src/scheduling/admission-controller.h@339 PS1, Line 339: void ReleaseQueryBackends( Needs a comment. We may also want to document the lifecycle of resource release in a section in the class comment above, since it's gotten a little more complex. http://gerrit.cloudera.org:8080/#/c/14104/1/be/src/scheduling/admission-controller.h@489 PS1, Line 489: void ReleaseQueryBackends( Needs a comment http://gerrit.cloudera.org:8080/#/c/14104/1/be/src/scheduling/admission-controller.h@816 PS1, Line 816: void UpdateHostStatsForQueryBackends( Needs a comment http://gerrit.cloudera.org:8080/#/c/14104/1/be/src/scheduling/admission-controller.cc File be/src/scheduling/admission-controller.cc: http://gerrit.cloudera.org:8080/#/c/14104/1/be/src/scheduling/admission-controller.cc@375 PS1, Line 375: schedule.coord_backend_mem_to_admit() : This pattern appears in a few places - might be worth making a helper function in backend_exec_params. http://gerrit.cloudera.org:8080/#/c/14104/1/tests/query_test/test_result_spooling.py File tests/query_test/test_result_spooling.py: http://gerrit.cloudera.org:8080/#/c/14104/1/tests/query_test/test_result_spooling.py@82 PS1, Line 82: class TestDedicatedCoordinator(CustomClusterTestSuite): Let's move this into the custom_cluster directory. run-custom-cluster-tests.sh assumes that they are in certain subdirectories. -- To view, visit http://gerrit.cloudera.org:8080/14104 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I88bb11e0ede7574568020e0277dd8ac8d2586dc9 Gerrit-Change-Number: 14104 Gerrit-PatchSet: 1 Gerrit-Owner: Sahil Takiar <stak...@cloudera.com> Gerrit-Reviewer: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Wed, 21 Aug 2019 00:25:36 +0000 Gerrit-HasComments: Yes
