Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/13078 )
Change subject: IMPALA-8446: Create a unit test for Admission Controller. ...................................................................... Patch Set 2: (4 comments) looks good, just a few nits http://gerrit.cloudera.org:8080/#/c/13078/2/be/src/scheduling/admission-controller-test.cc File be/src/scheduling/admission-controller-test.cc: http://gerrit.cloudera.org:8080/#/c/13078/2/be/src/scheduling/admission-controller-test.cc@31 PS2, Line 31: CURRENT_EXECUTABLE_PATH when does the SASL lib use this? http://gerrit.cloudera.org:8080/#/c/13078/2/be/src/scheduling/admission-controller-test.cc@139 PS2, Line 139: // Test that the pool configurations can be read correctly. : CheckPoolConfig(request_pool_service, "non-existent queue", 5, -1, 30000, true); : CheckPoolConfig(request_pool_service, QUEUE_A, 1, 100000L * 1024L * 1024L, 50, true); : CheckPoolConfig(request_pool_service, QUEUE_B, 5, -1, 600000, true); : CheckPoolConfig(request_pool_service, QUEUE_C, 10, 128L * 1024L * 1024L, 30000, true); this can be a separate test. http://gerrit.cloudera.org:8080/#/c/13078/2/be/src/scheduling/admission-controller-test.cc@182 PS2, Line 182: ASSERT_EQ(3, admission_controller.host_mem_reserved_.size()); : ASSERT_EQ(6000, admission_controller.host_mem_reserved_[HOST_1]); : ASSERT_EQ(5000, admission_controller.host_mem_reserved_[HOST_2]); nit: should we add the same set of checks before the call to UpdatePoolStats to make sure its empty? or will that be an overkill? http://gerrit.cloudera.org:8080/#/c/13078/2/be/src/scheduling/admission-controller-test.cc@188 PS2, Line 188: lock_guard<mutex> lock(admission_controller.admission_ctrl_lock_); actually other variables like host_mem_reserved_ and the function calls like CanAdmitRequest are all protected by this lock. Since this is a single thread test and all calls are synchronous, maybe we can just ignore holding the lock OR hold it everytime we use something that should be protected by it. -- To view, visit http://gerrit.cloudera.org:8080/13078 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8a840590b868f2df1a06f3f397b7b0fc2b29462c Gerrit-Change-Number: 13078 Gerrit-PatchSet: 2 Gerrit-Owner: Andrew Sherman <asher...@cloudera.com> Gerrit-Reviewer: Andrew Sherman <asher...@cloudera.com> Gerrit-Reviewer: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Tue, 23 Apr 2019 18:21:52 +0000 Gerrit-HasComments: Yes
