Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/10396 )
Change subject: IMPALA-3134: Support different proc mem limits among impalads for admission control checks ...................................................................... Patch Set 2: (10 comments) Looks good, had some minor comments. http://gerrit.cloudera.org:8080/#/c/10396/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/10396/2//COMMIT_MSG@14 PS2, Line 14: decisions. Can you mention that this assumes that the per-process memory limit does not change dynamically? Might not be obvious. http://gerrit.cloudera.org:8080/#/c/10396/2/be/src/scheduling/admission-controller.cc File be/src/scheduling/admission-controller.cc: http://gerrit.cloudera.org:8080/#/c/10396/2/be/src/scheduling/admission-controller.cc@49 PS2, Line 49: int64_t GetProcMemLimit() { This is dead code now http://gerrit.cloudera.org:8080/#/c/10396/2/be/src/scheduling/admission-controller.cc@152 PS2, Line 152: const string HOST_MEM_NOT_AVAILABLE = "Not enough memory available on host $0." Maybe this error message should include the total process memory limit, e.g. "$2/$3 was available" http://gerrit.cloudera.org:8080/#/c/10396/2/be/src/scheduling/admission-controller.cc@450 PS2, Line 450: Unnecessary vertical whitespace http://gerrit.cloudera.org:8080/#/c/10396/2/be/src/scheduling/admission-controller.cc@458 PS2, Line 458: Vertical whitespace http://gerrit.cloudera.org:8080/#/c/10396/2/be/src/scheduling/query-schedule.h File be/src/scheduling/query-schedule.h: http://gerrit.cloudera.org:8080/#/c/10396/2/be/src/scheduling/query-schedule.h@69 PS2, Line 69: // The process memory limit of this backend. Maybe mention that it's the value obtained from the statestore during scheduling. We don't expect it to change right now, but that may be important context if we ever want to change to dynamic process memory limits. http://gerrit.cloudera.org:8080/#/c/10396/2/tests/custom_cluster/test_admission_controller.py File tests/custom_cluster/test_admission_controller.py: http://gerrit.cloudera.org:8080/#/c/10396/2/tests/custom_cluster/test_admission_controller.py@429 PS2, Line 429: 1000 Can we set this lower? Is there a reason that it needs to sit in the queue for 1s? http://gerrit.cloudera.org:8080/#/c/10396/2/tests/custom_cluster/test_admission_controller.py@431 PS2, Line 431: def test_heterogeneous_proc_mem_limit(self, vector): Can we add a 3GB query that succeeds? E.g. with num_nodes=1, submitted to one of the 3GB nodes. http://gerrit.cloudera.org:8080/#/c/10396/2/tests/custom_cluster/test_admission_controller.py@433 PS2, Line 433: mem limits of each impalad""" Maybe add an extra sentence to explain the concept of the test. e.g. "Starts a cluster where the first impalad has a smaller proc mem limit than other impalads and run queries where admission/reject depends on the coordinator knowing the other impalad's mem limits". http://gerrit.cloudera.org:8080/#/c/10396/2/tests/custom_cluster/test_admission_controller.py@434 PS2, Line 434: # Choose a query that runs on all 3 backends. Are all these queries submitted to the first impalad? It's not clear if that's the intent. -- To view, visit http://gerrit.cloudera.org:8080/10396 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Idb72eee790cc17466bbfa82e30f369a65f2b060e Gerrit-Change-Number: 10396 Gerrit-PatchSet: 2 Gerrit-Owner: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Tue, 15 May 2018 17:44:11 +0000 Gerrit-HasComments: Yes
