Andrew Sherman has posted comments on this change. ( http://gerrit.cloudera.org:8080/21616 )
Change subject: IMPALA-12345: Add user quotas to Admission Control ...................................................................... Patch Set 4: (18 comments) Thanks for the review The main changes in this patchset beyond addressing review comments are: I had not accounted for AdmissionController being hosted in AdmissionD. To allow this I added a getHadoopGroups() implementation to RequestPool. This is a small duplication of code but it turns out to be hard and heavyweight to add a Frontend to AdmissionD. Update my new test_admission_controller.py tests to run in exhaustive mode where AdmissionController is hosted in AdmissionD - in this case the metrics look different. http://gerrit.cloudera.org:8080/#/c/21616/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21616/3//COMMIT_MSG@24 PS3, Line 24: two reasons: (1) queries can only be admitted from the front of the > nit: "as" is redundant at the beginning of the statement. Done http://gerrit.cloudera.org:8080/#/c/21616/3/be/src/common/global-flags.cc File be/src/common/global-flags.cc: http://gerrit.cloudera.org:8080/#/c/21616/3/be/src/common/global-flags.cc@193 PS3, Line 193: #ifdef SLOW_BUILD > nit: why add whitespace? Done http://gerrit.cloudera.org:8080/#/c/21616/3/be/src/scheduling/admission-controller-test.cc File be/src/scheduling/admission-controller-test.cc: http://gerrit.cloudera.org:8080/#/c/21616/3/be/src/scheduling/admission-controller-test.cc@404 PS3, Line 404: EXPECT_FALSE(false) << "unknown outcome"; > This should be EXPECT_FALSE. clang-tidy flags this because it doesn't exit, Done http://gerrit.cloudera.org:8080/#/c/21616/3/be/src/scheduling/admission-controller.h File be/src/scheduling/admission-controller.h: http://gerrit.cloudera.org:8080/#/c/21616/3/be/src/scheduling/admission-controller.h@a1155 PS3, Line 1155: > Removed because it's unused? Yes http://gerrit.cloudera.org:8080/#/c/21616/3/be/src/scheduling/admission-controller.h@535 PS3, Line 535: typedef std::map<std::string, int64_t> UserLoads; > Where does the definition for int64 come from? I thought only int64_t was s Thanks, done http://gerrit.cloudera.org:8080/#/c/21616/3/be/src/scheduling/admission-controller.cc File be/src/scheduling/admission-controller.cc: http://gerrit.cloudera.org:8080/#/c/21616/3/be/src/scheduling/admission-controller.cc@770 PS3, Line 770: if (!user.empty()) { > Is this guaranteed to be symmetric with the condition in AdmitQuaryAndMemor The idea is that RunningQuery.query is set only if user quotas are configured. So yes it is supposed to be symmetrical but hard to prove. http://gerrit.cloudera.org:8080/#/c/21616/3/be/src/scheduling/admission-controller.cc@861 PS3, Line 861: for (const auto& load : loads) { > nit: can optimize this a bit with something like https://stackoverflow.com/ Thanks, yes, original code is too cautious http://gerrit.cloudera.org:8080/#/c/21616/3/be/src/scheduling/admission-controller.cc@882 PS3, Line 882: } > Usual pattern here is to use find Thanks http://gerrit.cloudera.org:8080/#/c/21616/3/be/src/scheduling/admission-controller.cc@1161 PS3, Line 1161: return true; > nit: with most functions, I tend to check if they can be const, or implemen Good advice, thanks http://gerrit.cloudera.org:8080/#/c/21616/3/be/src/scheduling/admission-controller.cc@1177 PS3, Line 1177: quota_exceeded_reason, &key_matched)) { > Why VLOG_ROW rather than VLOG_QUERY? Done http://gerrit.cloudera.org:8080/#/c/21616/3/be/src/scheduling/admission-controller.cc@1205 PS3, Line 1205: user_limit = it->second; > nit: I'd prefer to avoid copying 'user' here. Done http://gerrit.cloudera.org:8080/#/c/21616/3/be/src/scheduling/admission-controller.cc@1213 PS3, Line 1213: *key_matched = true; > const string& to avoid the copy? Done http://gerrit.cloudera.org:8080/#/c/21616/3/be/src/scheduling/admission-controller.cc@1611 PS3, Line 1611: > What led to this change? names.h has `using std::move`. This was to make clion shut up. I'll restore the old code http://gerrit.cloudera.org:8080/#/c/21616/3/be/src/scheduling/admission-controller.cc@1940 PS3, Line 1940: void AdmissionController::UpdatePoolStats( > Could this use ROOT_POOL? Done http://gerrit.cloudera.org:8080/#/c/21616/3/be/src/scheduling/admission-controller.cc@2190 PS3, Line 2190: // process executor groups in alphanumerically sorted order. > Why this change? It doesn't exceed the 90 character line limit. Yeah ask clang-format I've rolled this back http://gerrit.cloudera.org:8080/#/c/21616/3/be/src/scheduling/admission-controller.cc@2418 PS3, Line 2418: ClusterMembershipMgr::SnapshotPtr membership_snapshot = > Little bit of refactoring? Yes, separated out so that unit tests could call TryDequeue() to make a single pass through the dequeue logic. Actually in the end the test doesn']t do much but this seems a useful refactoring. http://gerrit.cloudera.org:8080/#/c/21616/3/fe/src/test/resources/fair-scheduler-test.xml File fe/src/test/resources/fair-scheduler-test.xml: http://gerrit.cloudera.org:8080/#/c/21616/3/fe/src/test/resources/fair-scheduler-test.xml@4 PS3, Line 4: <userQueryLimit>userB 6</userQueryLimit> > Why'd you land on this format, rather than something more structured? It co Ha the answer is because that was easy in the Java code. I'm going to look at this some more because yes your structure is better. http://gerrit.cloudera.org:8080/#/c/21616/3/java/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/AllocationConfiguration.java File java/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/AllocationConfiguration.java: http://gerrit.cloudera.org:8080/#/c/21616/3/java/yarn-extras/src/main/java/org/apache/impala/yarn/server/resourcemanager/scheduler/fair/AllocationConfiguration.java@21 PS3, Line 21: import java.util.Collections; > This isn't the first time we've made changes to yarn-extras source code, bu I think we've forked this code, so we own it. But I'm open to suggestions as to how to mark it -- To view, visit http://gerrit.cloudera.org:8080/21616 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4c33f3f2427db57fb9b6c593a4b22d5029549b41 Gerrit-Change-Number: 21616 Gerrit-PatchSet: 4 Gerrit-Owner: Andrew Sherman <[email protected]> Gerrit-Reviewer: Andrew Sherman <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Comment-Date: Fri, 30 Aug 2024 00:09:30 +0000 Gerrit-HasComments: Yes
