impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Marcel Kornacker (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-2550: Switch to per-query exec rpc
Date Thu, 20 Apr 2017 01:39:39 GMT
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 <marcel@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Henry Robinson <henry@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <marcel@cloudera.com>
Gerrit-Reviewer: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message