Sahil Takiar has posted comments on this change. ( http://gerrit.cloudera.org:8080/16411 )
Change subject: IMPALA-9930 (part 1): Initial refactor for admission control service ...................................................................... Patch Set 2: (6 comments) http://gerrit.cloudera.org:8080/#/c/16411/2/be/src/scheduling/admission-control-client.h File be/src/scheduling/admission-control-client.h: http://gerrit.cloudera.org:8080/#/c/16411/2/be/src/scheduling/admission-control-client.h@35 PS2, Line 35: AdmissionControlClient(const TUniqueId& query_id); : : Status SubmitForAdmission(const AdmissionController::AdmissionRequest& request, : std::unique_ptr<QuerySchedulePB>* schedule_result); : : void ReleaseQuery(int64_t peak_mem_consumption); : : void ReleaseQueryBackends(const std::vector<NetworkAddressPB>& host_addr); : : void CancelAdmission(); nit: docs http://gerrit.cloudera.org:8080/#/c/16411/2/be/src/scheduling/admission-control-client.h@47 PS2, Line 47: UniqueIdPB query_id_; nit: docs http://gerrit.cloudera.org:8080/#/c/16411/2/be/src/scheduling/admission-control-client.h@51 PS2, Line 51: bool use_local_; would it be better to model AdmissionControlClient as an purely virtual class (e.g. an interface) and then have two implementations: LocalAdmissionControlClient and RemoteAdmissionControlClient. seems like it would help organize the code a bit better. http://gerrit.cloudera.org:8080/#/c/16411/2/be/src/scheduling/admission-controller.cc File be/src/scheduling/admission-controller.cc: http://gerrit.cloudera.org:8080/#/c/16411/2/be/src/scheduling/admission-controller.cc@1527 PS2, Line 1527: membsrship nit: typo http://gerrit.cloudera.org:8080/#/c/16411/2/be/src/util/sharded-query-map-util.h File be/src/util/sharded-query-map-util.h: http://gerrit.cloudera.org:8080/#/c/16411/2/be/src/util/sharded-query-map-util.h@61 PS2, Line 61: Status Get(const K& key, V* value); nit: docs http://gerrit.cloudera.org:8080/#/c/16411/2/be/src/util/sharded-query-map-util.h@165 PS2, Line 165: : template <typename K, typename V> : Status GenericShardedQueryMap<K, V>::Add(const K& query_id, const V& obj) { : GenericScopedShardedMapRef<K, V> map_ref(query_id, this); : DCHECK(map_ref.get() != nullptr); : : auto entry = map_ref->find(query_id); : if (entry != map_ref->end()) { : // There shouldn't be an active query with that same id. : // (query_id is globally unique) : return Status(ErrorMsg(TErrorCode::INTERNAL_ERROR, : strings::Substitute("query id $0 already exists", PrintId(query_id)))); : } : map_ref->insert(make_pair(query_id, obj)); : return Status::OK(); : } : : template <typename K, typename V> : Status GenericShardedQueryMap<K, V>::Get(const K& query_id, V* obj) { : GenericScopedShardedMapRef<K, V> map_ref(query_id, this); : DCHECK(map_ref.get() != nullptr); : : auto entry = map_ref->find(query_id); : if (entry == map_ref->end()) { : Status err = Status::Expected(TErrorCode::INVALID_QUERY_HANDLE, PrintId(query_id)); : VLOG(1) << err.GetDetail(); : return err; : } : *obj = entry->second; : return Status::OK(); : } : : template <typename K, typename V> : Status GenericShardedQueryMap<K, V>::Delete(const K& query_id) { : GenericScopedShardedMapRef<K, V> map_ref(query_id, this); : DCHECK(map_ref.get() != nullptr); : auto entry = map_ref->find(query_id); : if (entry == map_ref->end()) { : Status err = Status::Expected(TErrorCode::INVALID_QUERY_HANDLE, PrintId(query_id)); : VLOG(1) << err.GetDetail(); : return err; : } : map_ref->erase(entry); : return Status::OK(); : } should all of this (and possibly more) be moved into a .cc file? I remember there was a include-what-you-use report that complained adding this code to the header file was blowing up some .h files -- To view, visit http://gerrit.cloudera.org:8080/16411 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7974a979cf05ed569f31e1ab20694e29fd3e4508 Gerrit-Change-Number: 16411 Gerrit-PatchSet: 2 Gerrit-Owner: Thomas Tauber-Marshall <tmarsh...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Reviewer: Sahil Takiar <stak...@cloudera.com> Gerrit-Comment-Date: Mon, 14 Sep 2020 23:25:39 +0000 Gerrit-HasComments: Yes
