Sahil Takiar has posted comments on this change. ( http://gerrit.cloudera.org:8080/15961 )
Change subject: IMPALA-9692 (part 3): Model QuerySchedule as a protobuf ...................................................................... Patch Set 4: (13 comments) I think I got through most of the query-scheduler and scheduler changes. probably tackle the admission-controller class next. http://gerrit.cloudera.org:8080/#/c/15961/4/be/src/scheduling/admission-controller.cc File be/src/scheduling/admission-controller.cc: http://gerrit.cloudera.org:8080/#/c/15961/4/be/src/scheduling/admission-controller.cc@899 PS4, Line 899: std:: nit: we omit the std namespace in .cc files, right? http://gerrit.cloudera.org:8080/#/c/15961/4/be/src/scheduling/query-schedule.h File be/src/scheduling/query-schedule.h: http://gerrit.cloudera.org:8080/#/c/15961/4/be/src/scheduling/query-schedule.h@56 PS4, Line 56: query_schedule_pb_ do you mean backend_exec_params? http://gerrit.cloudera.org:8080/#/c/15961/4/be/src/scheduling/query-schedule.h@58 PS4, Line 58: pb 'pb' is a bit of an add name for this, why not just 'backend_exec_params_pb_'? http://gerrit.cloudera.org:8080/#/c/15961/4/be/src/scheduling/query-schedule.h@108 PS4, Line 108: BackendExecParams swapped to 'BackendExecParamsPB' right? http://gerrit.cloudera.org:8080/#/c/15961/4/be/src/scheduling/query-schedule.h@114 PS4, Line 114: pb same comment as above, name could be revised. http://gerrit.cloudera.org:8080/#/c/15961/4/be/src/scheduling/query-schedule.h@124 PS4, Line 124: . so we shouldn't use this class (or any of the related structs) outside the 'scheduling' folder, right? might be good to mention that. http://gerrit.cloudera.org:8080/#/c/15961/4/be/src/scheduling/query-schedule.h@130 PS4, Line 130: QuerySchedule it would be nice if this class or QuerySchedulePB was renamed. QuerySchedule and QuerySchedulePB seem to encapsulate different things, and using the same name is a bit confusing. http://gerrit.cloudera.org:8080/#/c/15961/4/be/src/scheduling/query-schedule.h@193 PS4, Line 193: PerBackendExecParams& per_backend_exec_params() { return per_backend_exec_params_; } why is non-const access to per_backend_exec_params_ and fragment_exec_params_ necessary now? http://gerrit.cloudera.org:8080/#/c/15961/4/be/src/scheduling/query-schedule.h@285 PS4, Line 285: /// Contains the results of scheduling that will be sent back to the coordinator. mention that ownership is transferred to the coordinator after submission has completed http://gerrit.cloudera.org:8080/#/c/15961/4/be/src/scheduling/query-schedule.cc File be/src/scheduling/query-schedule.cc: http://gerrit.cloudera.org:8080/#/c/15961/4/be/src/scheduling/query-schedule.cc@86 PS4, Line 86: for (int i = 0; i < fragments_.size(); ++i) can u still use the 'for ([var] : fragments)' pattern? http://gerrit.cloudera.org:8080/#/c/15961/4/be/src/scheduling/query-schedule.cc@153 PS4, Line 153: for (const auto& entry : per_backend_exec_params_) { is this just general cleanup? I guess per_backend_exec_params_ is smaller than fragment_exec_params_ right? http://gerrit.cloudera.org:8080/#/c/15961/4/be/src/scheduling/query-schedule.cc@201 PS4, Line 201: GetBackendExecParams seems more like 'GetOrCreateBackendExecParams' or 'LoadBackendExecParams' method, since it creates one if it if it does not already exist. http://gerrit.cloudera.org:8080/#/c/15961/4/be/src/scheduling/scheduler.cc File be/src/scheduling/scheduler.cc: http://gerrit.cloudera.org:8080/#/c/15961/4/be/src/scheduling/scheduler.cc@300 PS4, Line 300: ScanRangesPB& scan_ranges = : (*instance_params.pb.mutable_per_node_scan_ranges())[entry.first]; why do u have to do this lookup? how was it working before? -- To view, visit http://gerrit.cloudera.org:8080/15961 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1db64e72f84604b1d8ac24e0bdd4ad6bedd6bcd9 Gerrit-Change-Number: 15961 Gerrit-PatchSet: 4 Gerrit-Owner: Thomas Tauber-Marshall <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Sahil Takiar <[email protected]> Gerrit-Comment-Date: Tue, 16 Jun 2020 18:02:55 +0000 Gerrit-HasComments: Yes
