Thomas Tauber-Marshall 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 3: (8 comments) http://gerrit.cloudera.org:8080/#/c/16411/2/be/src/rpc/sidecar-util.h File be/src/rpc/sidecar-util.h: http://gerrit.cloudera.org:8080/#/c/16411/2/be/src/rpc/sidecar-util.h@83 PS2, Line 83: Status > In the original code in Coordinator::BackendState::ExecAsync, this Status w I don't think so - Expected() is for common errors (which this shouldn't be) and just prevents logging of a stack trace (which we actually need now, since this function could be called from multiple places and the error doesn't have any way to indicate what sidecar is too big). 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: // Creates a new AdmissionControlClient and returns it in 'client'. : static void Create( : const TUniqueId& query_id, std::unique_ptr<AdmissionControlClient>* client); : : // Called to schedule and admit the query. Blocks until an admission decision is made. : virtual Status SubmitForAdmission(const AdmissionController::AdmissionRequest& request, : std::unique_ptr<QuerySchedulePB>* schedule_result) = 0; : : // Called when the query has completed to release all of its resources. : virtual void ReleaseQue > nit: docs Done http://gerrit.cloudera.org:8080/#/c/16411/2/be/src/scheduling/admission-control-client.h@47 PS2, Line 47: // for the query on those backends. > nit: docs Done http://gerrit.cloudera.org:8080/#/c/16411/2/be/src/scheduling/admission-control-client.h@51 PS2, Line 51: virtual void CancelAdmission() = 0; > would it be better to model AdmissionControlClient as an purely virtual cla Done 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@1525 PS2, Line 1525: It' > nit: It's Done http://gerrit.cloudera.org:8080/#/c/16411/2/be/src/scheduling/admission-controller.cc@1527 PS2, Line 1527: membership > nit: typo Done 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: // Returns the value associated with 'key' in 'value', returning an error if 'key' > nit: docs Done http://gerrit.cloudera.org:8080/#/c/16411/2/be/src/util/sharded-query-map-util.h@165 PS2, Line 165: : GenericScopedShardedMapRef<TUniqueId, T>(query_id, sharded_map) {} : }; : : } // namespace impala : : : : : : : : : : : : : : : : : : : : : : : : : : : : : : : : : : : : : : : : : > should all of this (and possibly more) be moved into a .cc file? I remember Done -- 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: 3 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-Reviewer: Thomas Tauber-Marshall <tmarsh...@cloudera.com> Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com> Gerrit-Comment-Date: Thu, 24 Sep 2020 01:19:23 +0000 Gerrit-HasComments: Yes
