Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/21616 )
Change subject: IMPALA-12345: Add user quotas to Admission Control ...................................................................... Patch Set 3: (16 comments) 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? http://gerrit.cloudera.org:8080/#/c/21616/3/be/src/scheduling/admission-controller.h@535 PS3, Line 535: typedef std::map<std::string, int64> UserLoads; Where does the definition for int64 come from? I thought only int64_t was standard. I guess gutil does a typedef int64_t to int64; I'd rather not rely on it. 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 AdmitQuaryAndMemory? Not clear to me why they would be. http://gerrit.cloudera.org:8080/#/c/21616/3/be/src/scheduling/admission-controller.cc@811 PS3, Line 811: agg_user_loads_.decrement(user); nit: could improve DecrementCount to return the resulting value after decrement, which would let you skip doing a separate lookup. http://gerrit.cloudera.org:8080/#/c/21616/3/be/src/scheduling/admission-controller.cc@861 PS3, Line 861: if (loads_.count(load.first)) { nit: can optimize this a bit with something like https://stackoverflow.com/a/1409465, i.e. r = loads_.insert(load.first, load.second) if (!r.second) { // insert failed because there was already a value r.first->second += load.second } Or maybe this could be simpler. You already rely on loads_[key] == 0 if key doesn't exist in IncrementCount, which seems to be guaranteed according to https://en.cppreference.com/w/cpp/container/map/operator_at ( specifically, zero-initialized). So you could just do loads_[load.first] += load.second; http://gerrit.cloudera.org:8080/#/c/21616/3/be/src/scheduling/admission-controller.cc@882 PS3, Line 882: // FIXME C++20: use contains(). Usual pattern here is to use find if (auto it = loads_.find(key); it != loads_.end()) { return it->second; } return 0; http://gerrit.cloudera.org:8080/#/c/21616/3/be/src/scheduling/admission-controller.cc@1161 PS3, Line 1161: bool AdmissionController::HasSufficientPoolQuotas(const ScheduleState& state, nit: with most functions, I tend to check if they can be const, or implemented as a local static function (rather than a class member), to limit the impact we need to check when making changes. This function and its helpers don't seem like they use any class instance variables, so they could at least be const. http://gerrit.cloudera.org:8080/#/c/21616/3/be/src/scheduling/admission-controller.cc@1177 PS3, Line 1177: VLOG_ROW << "user " << user << " passes quota check as user rule is matched"; Why VLOG_ROW rather than VLOG_QUERY? http://gerrit.cloudera.org:8080/#/c/21616/3/be/src/scheduling/admission-controller.cc@1205 PS3, Line 1205: string user_for_limits = use_wildcard ? "*" : user; nit: I'd prefer to avoid copying 'user' here. http://gerrit.cloudera.org:8080/#/c/21616/3/be/src/scheduling/admission-controller.cc@1213 PS3, Line 1213: string format = use_wildcard ? USER_WILDCARD_QUOTA_EXCEEDED : USER_QUOTA_EXCEEDED; const string& to avoid the copy? http://gerrit.cloudera.org:8080/#/c/21616/3/be/src/scheduling/admission-controller.cc@1611 PS3, Line 1611: *schedule_result = std::move(queue_node->admitted_schedule->query_schedule_pb()); What led to this change? names.h has `using std::move`. http://gerrit.cloudera.org:8080/#/c/21616/3/be/src/scheduling/admission-controller.cc@1940 PS3, Line 1940: return request_pool_service_->GetPoolConfig("root", root_config); Could this use ROOT_POOL? http://gerrit.cloudera.org:8080/#/c/21616/3/be/src/scheduling/admission-controller.cc@2190 PS3, Line 2190: ExecutorGroup("all-coordinators"); Why this change? It doesn't exceed the 90 character line limit. http://gerrit.cloudera.org:8080/#/c/21616/3/be/src/scheduling/admission-controller.cc@2418 PS3, Line 2418: TryDequeue(); Little bit of 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 could have looked more like <userQueryLimit> <user>userB</user> <limit>6</limit> </userQueryLimit> and <groupQueryLimit> <group>group1</group> <group>group2</group> <limit>1</limit> </groupQueryLimit> 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, but it's been a long time (last I see is 2018). I assume that's fine, but we might want to add some comments to our additions to make it easier to trace provenance to the original source. -- 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: 3 Gerrit-Owner: Andrew Sherman <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Comment-Date: Tue, 06 Aug 2024 21:14:44 +0000 Gerrit-HasComments: Yes
