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 6: (13 comments) http://gerrit.cloudera.org:8080/#/c/15961/6//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/15961/6//COMMIT_MSG@31 PS6, Line 31: evenaul nit: typo http://gerrit.cloudera.org:8080/#/c/15961/6//COMMIT_MSG@42 PS6, Line 42: get nit typo http://gerrit.cloudera.org:8080/#/c/15961/4/be/src/runtime/coordinator-backend-state.cc File be/src/runtime/coordinator-backend-state.cc: http://gerrit.cloudera.org:8080/#/c/15961/4/be/src/runtime/coordinator-backend-state.cc@a95 PS4, Line 95: : > Yeah, we no longer include the host with the FInstanceExecParams that we se makes sense http://gerrit.cloudera.org:8080/#/c/15961/6/be/src/runtime/exec-params.h File be/src/runtime/exec-params.h: http://gerrit.cloudera.org:8080/#/c/15961/6/be/src/runtime/exec-params.h@37 PS6, Line 37: ExecParams we might actually want to rename this to something like QueryExecParams, otherwise its a bit confusing since there are a bunch of existing classes with ExecParams in the name: BackendExecParams, FInstanceExecParams, FragmentExecParams, PerBackendExecParams, etc. http://gerrit.cloudera.org:8080/#/c/15961/6/be/src/scheduling/admission-controller.h File be/src/scheduling/admission-controller.h: http://gerrit.cloudera.org:8080/#/c/15961/6/be/src/scheduling/admission-controller.h@724 PS6, Line 724: memorty nit: typo http://gerrit.cloudera.org:8080/#/c/15961/6/be/src/scheduling/admission-controller.h@745 PS6, Line 745: running_queries_ mention this is guarded by admission_ctrl_lock_? http://gerrit.cloudera.org:8080/#/c/15961/4/be/src/scheduling/admission-controller.h File be/src/scheduling/admission-controller.h: http://gerrit.cloudera.org:8080/#/c/15961/4/be/src/scheduling/admission-controller.h@341 PS4, Line 341: std::unique_ptr<QuerySchedulePB>* schedule_result > Definitely agree that the eventual AdmitQuery RPC will take an AdmitQueryRe no its fine for now since ur planning on making the change in a future patch; just wanted to bring it up 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: queue > I fixed the instances in this file. makes sense, thanks for making the change http://gerrit.cloudera.org:8080/#/c/15961/4/be/src/scheduling/cluster-membership-mgr.cc File be/src/scheduling/cluster-membership-mgr.cc: http://gerrit.cloudera.org:8080/#/c/15961/4/be/src/scheduling/cluster-membership-mgr.cc@344 PS4, Line 344: const UniqueIdPB& backend_id > Previously, Coordinator::BackendState had access to its BackendDescriptorPB ack makes sense, thanks http://gerrit.cloudera.org:8080/#/c/15961/4/be/src/scheduling/cluster-membership-mgr.cc@352 PS4, Line 352: INFO > Yeah, I think this could happen if: makes sense, perhaps a better INFO message would be "Did not blacklist ... because it was already removed from the cluster membership." 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@58 PS4, Line 58: er > That's fair. Of course I was trying to avoid getting too verbose/repetitive I think that makes sense, I like the 'ScheduleState' approach. yeah, its up to you whether you want to make the change in this patch or a follow up. 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: *scan_ranges.mutable_scan_ranges() = {entry.second.begin(), entry.second.end()}; : } > The structure has changed, because thrift allows maps with value types that okay - might be worth re-factoring into a static helper method to make it a bit cleaner. I think the code change at the end of CreateCollocatedAndScanInstances has similar logic, so perhaps that can be re-factored to use the new method as well. http://gerrit.cloudera.org:8080/#/c/15961/6/common/protobuf/admission_control_service.proto File common/protobuf/admission_control_service.proto: http://gerrit.cloudera.org:8080/#/c/15961/6/common/protobuf/admission_control_service.proto@39 PS6, Line 39: // 0-based ordinal number of this particular instance. This is within its fragment, not : // query-wide, so eg. there will be one instance '0' for each fragment. still a bit confused about this represents, but I think this is basically the 'idx' of the fragment instance, similar to 'fragment_idx' for fragments? for example, say there are 2 fragments idx = [0, 1] and each fragment has 5 instances. then fragment idx = 0 will have instances with per_fragment_instance_idx = [0, 4] and fragment idx = 1 will have per_fragment_instance_idx = [0, 4] as well? -- 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: 6 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-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-Comment-Date: Tue, 23 Jun 2020 21:25:22 +0000 Gerrit-HasComments: Yes
