Thomas Tauber-Marshall 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 7: (10 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: eventua > nit: typo Done http://gerrit.cloudera.org:8080/#/c/15961/6//COMMIT_MSG@42 PS6, Line 42: get > nit typo Seems right to me? 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: > we might actually want to rename this to something like QueryExecParams, ot Done http://gerrit.cloudera.org:8080/#/c/15961/4/be/src/runtime/exec-params.h File be/src/runtime/exec-params.h: http://gerrit.cloudera.org:8080/#/c/15961/4/be/src/runtime/exec-params.h@73 PS4, Line 73: > What I had in mind is if you know the total number of indices, then you can Done 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: memory > nit: typo Done http://gerrit.cloudera.org:8080/#/c/15961/6/be/src/scheduling/admission-controller.h@745 PS6, Line 745: > mention this is guarded by admission_ctrl_lock_? Done 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@352 PS4, Line 352: INFO > makes sense, perhaps a better INFO message would be "Did not blacklist ... Done 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 > I think that makes sense, I like the 'ScheduleState' approach. yeah, its up Great, I'll do it in 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: } : } else if (ContainsNode(fragment.plan, TPlanNodeType::UNION_NODE) > okay - might be worth re-factoring into a static helper method to make it a Done 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 t Right -- 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: 7 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: Fri, 26 Jun 2020 15:45:20 +0000 Gerrit-HasComments: Yes
