Joe McDonnell 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: (4 comments) 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: std::unordered_map<int32_t, const TPlanFragment&> fragments_; Nit: In the coordinator, we treat this like a vector, relying on the fragment idxs being contiguous. This could be a vector. There are obstacles. You can't use const references and you need to be able to size it up front. A vector would let you have a GetFragments() function return a const vector<const TPlanFragment*>& that Coordinator::InitFragmentStats() can iterate over. 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@1607 PS4, Line 1607: entry.second.pb->is_coord_backend() ? : schedule->coord_backend_mem_to_admit() : : schedule->per_backend_mem_to_admit(); Nit: You could use GetMemToAdmit() here 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@82 PS4, Line 82: BackendExecParams Nit: BackendExecParamsPB http://gerrit.cloudera.org:8080/#/c/15961/4/be/src/scheduling/query-schedule.h@320 PS4, Line 320: /// Map from fragment idx to references into the 'request_'. : std::unordered_map<int32_t, const TPlanFragment&> fragments_; Nit: Same thought I mentioned on ExecParams, the fragment indexes are expected to be contiguous. This could be a vector. -- 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: Wed, 17 Jun 2020 16:17:51 +0000 Gerrit-HasComments: Yes
