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: (11 comments) mostly typos and questions so far, still trying to understand this part of the code http://gerrit.cloudera.org:8080/#/c/15961/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/15961/4//COMMIT_MSG@21 PS4, Line 21: This has : been replaced with the ExecParams class the part I don't fully understand is that doesn't the regular QuerySchedule object still exist? and it still has a reference to TExecRequest? so is there a specific reason the ExecParams class needs to exist? http://gerrit.cloudera.org:8080/#/c/15961/4//COMMIT_MSG@31 PS4, Line 31: AdmissionController::ReleaseQuery() and ReleaseQueryBackend() now : take a query id as a parameter instead of a QuerySchedule. the reason we do this is because ReleaseQuery() and ReleaseQueryBackend() will become RPCs into the new admission control service, right? and we don't want to serialize the entire QuerySchedule for each RPC call? http://gerrit.cloudera.org:8080/#/c/15961/4//COMMIT_MSG@43 PS4, Line 43: know 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: : intentionally removed? 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@51 PS4, Line 51: Return a references nit: typo http://gerrit.cloudera.org:8080/#/c/15961/4/be/src/runtime/exec-params.cc File be/src/runtime/exec-params.cc: http://gerrit.cloudera.org:8080/#/c/15961/4/be/src/runtime/exec-params.cc@32 PS4, Line 32: exer_equest_ nit: typo http://gerrit.cloudera.org:8080/#/c/15961/4/be/src/runtime/exec-params.cc@32 PS4, Line 32: refernces nit: typo http://gerrit.cloudera.org:8080/#/c/15961/4/be/src/runtime/exec-params.cc@48 PS4, Line 48: plan_exec_info[0].fragments[0] is it worth adding some DCHECKs to make sure both of these vectors have at least one element in them? 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 why was this changed to use a backend_id instead of a be_desc? just curious http://gerrit.cloudera.org:8080/#/c/15961/4/be/src/scheduling/cluster-membership-mgr.cc@352 PS4, Line 352: INFO should this ever happen? is there a reason it can't be DFATAL? http://gerrit.cloudera.org:8080/#/c/15961/4/common/protobuf/admission_control_service.proto File common/protobuf/admission_control_service.proto: http://gerrit.cloudera.org:8080/#/c/15961/4/common/protobuf/admission_control_service.proto@39 PS4, Line 39: not typo? -- 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: Mon, 15 Jun 2020 16:54:12 +0000 Gerrit-HasComments: Yes
