Marcel Kornacker has posted comments on this change. Change subject: IMPALA-2550: Switch to per-query exec rpc ......................................................................
Patch Set 6: (12 comments) http://gerrit.cloudera.org:8080/#/c/6535/4/be/src/runtime/coordinator-backend-state.cc File be/src/runtime/coordinator-backend-state.cc: Line 161: NotifyBarrierOnExit notifier(exec_complete_barrier); > long line Done PS4, Line 184: : if (!rpc_status.ok()) { > how is this out of scope ? it's general cleanup. this patch already contains a good deal of general cleanup, in addition to the actual logic changes for the exec rpc call, so i'll leave this particular piece of cleanup for later. PS4, Line 370: > long line Done PS4, Line 379: tal_split_size_(0), > Not needed ? Done in UpdateExecStats(), right ? Done PS4, Line 391: > *done = IsDone(); Done PS4, Line 434: > I don't see how this can spam the log if it's not expected to occur frequen this exact line will be changed in the very near future as part of switch to krpc. i don't think it's worth polishing it now. (in particular, this line will disappear.) PS4, Line 444: > magic constant ? #define or constexpr ? see previous comment about rpc retry. the new rpc stack abstracts that away anyway. Line 463: const string& root_profile_name, int num_instances, ObjectPool* obj_pool) > long line Done http://gerrit.cloudera.org:8080/#/c/6535/4/be/src/runtime/coordinator-backend-state.h File be/src/runtime/coordinator-backend-state.h: PS4, Line 45: p > the hierarchy fragment -> fragment instance is already well-documented else Done http://gerrit.cloudera.org:8080/#/c/6535/6/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: PS6, Line 408: // TODO: rename this metric > Correct. Done PS6, Line 885: TODO: do we need this error propagation path, in addition to the status report : // we'll get anyway? > It reports by making an RPC, which can be delayed for any one of a number o Done http://gerrit.cloudera.org:8080/#/c/6535/4/be/src/runtime/query-state.cc File be/src/runtime/query-state.cc: PS4, Line 194: query_ctx().c > Yes, it's used in the line below but it isn't added to some status object s Done -- To view, visit http://gerrit.cloudera.org:8080/6535 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I20769e420711737b6b385c744cef4851cee3facd Gerrit-PatchSet: 6 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Marcel Kornacker <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
