Marcel Kornacker has posted comments on this change. Change subject: IMPALA-2550: Switch to per-query exec rpc ......................................................................
Patch Set 8: (22 comments) http://gerrit.cloudera.org:8080/#/c/6535/8/be/src/runtime/coordinator-backend-state.cc File be/src/runtime/coordinator-backend-state.cc: PS8, Line 80: insert( > Please consider using emplace() instead. Done PS8, Line 96: ctx > finstance_ctx could be a more meaningful name. Done PS8, Line 104: TPlanFragmentCtx& ctx > May be fragment_ctx is a more meaningful name to distinguish from the TFrag Done Line 125: // instance separately > Will fixing IMPALA-3548 make this code snippet unnecessary ? not sure. PS8, Line 126: DCHECK > DCHECK_EQ() Done PS8, Line 136: // TODO: comment > Missing comment ? Done Line 226: bool Coordinator::BackendState::IsDoneInternal() const { > Consider adding inline. Done Line 261: if (instance_exec_status.done) --num_remaining_instances_; > DCHECK_GT(num_remaining_instances_, 0); before decrementing it. Done Line 299: int64_t completion_time = instance_stats.stopwatch_.ElapsedTime(); > Not sure if it's paranoid but should we have DCHECK_GT(completion_time, 0); Done http://gerrit.cloudera.org:8080/#/c/6535/8/be/src/runtime/coordinator-backend-state.h File be/src/runtime/coordinator-backend-state.h: PS8, Line 63: const DebugOptions& debug_options > Please add comment for this parameter. Done PS8, Line 81: // > /// Done PS8, Line 123: int per_fragment_instance_idx() const { return exec_params_.per_fragment_instance_idx; } > nit: long line Done http://gerrit.cloudera.org:8080/#/c/6535/8/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: PS8, Line 228: new FragmentStats(avg_profile_name, root_profile_name, num_instances, obj_pool())); > nit: long line Done PS8, Line 272: if (!fragment.__isset.plan) > Just curious under what circumstances will this be true ? i think if it's just a 'select 1'. PS8, Line 330: // source > Source plan node of the filter. Done PS8, Line 354: target > Target plan node of the filter Done http://gerrit.cloudera.org:8080/#/c/6535/8/be/src/runtime/coordinator.h File be/src/runtime/coordinator.h: PS8, Line 210: boost::unordered_map<TPlanNodeId, int> node_id_to_idx_map; > Should this be private ? Same for lock. made the struct private http://gerrit.cloudera.org:8080/#/c/6535/8/be/src/runtime/query-state.cc File be/src/runtime/query-state.cc: PS8, Line 108: Status status; > not used ? Done PS8, Line 297: insert > emplace() Done http://gerrit.cloudera.org:8080/#/c/6535/8/be/src/runtime/runtime-filter-bank.cc File be/src/runtime/runtime-filter-bank.cc: PS8, Line 102: VLOG_QUERY << "SendFilter()"; > remove ? Done http://gerrit.cloudera.org:8080/#/c/6535/8/be/src/runtime/runtime-state.cc File be/src/runtime/runtime-state.cc: PS8, Line 102: if (query_ctx().request_pool.empty()) { : const_cast<TQueryCtx&>(query_ctx()).request_pool = "test-pool"; : } > This seems odd. Why don't we just modify the test to set up query_ctx corre if you know it's a test, why not set it here? http://gerrit.cloudera.org:8080/#/c/6535/8/be/src/runtime/runtime-state.h File be/src/runtime/runtime-state.h: Line 85: /// installs desc_tbl, if set. If query_ctx.request_pool isn't set, sets it to "test-pool". > This seems odd. Can we modify the test to set up query_ctx.requet_pool corr the tests don't really care about this (unless they tests for it, and then they set it themselves), so i think it's better to do it here. -- 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: 8 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
