impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michael Ho (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-2550: Switch to per-query exec rpc
Date Thu, 13 Apr 2017 22:30:49 GMT
Michael Ho has posted comments on this change.

Change subject: IMPALA-2550: Switch to per-query exec rpc
......................................................................


Patch Set 4:

(32 comments)

Did a quick pass. Mostly formatting comments for now. Seems to be quite a bit of debugging
logging and #if 0 left in the code.

http://gerrit.cloudera.org:8080/#/c/6535/4/be/src/runtime/coordinator-backend-state.cc
File be/src/runtime/coordinator-backend-state.cc:

Line 64:   VLOG_QUERY << "BS::Init(): query_id=" << PrintId(query_id_) <<
" state_idx=" << state_idx_ << " host=" << host_;
long line


Line 161:   VLOG_QUERY << "BS::Exec(): query_id=" << PrintId(query_id_) <<
" state_idx=" << state_idx_ << " host=" << host_;
long line


PS4, Line 208: #if 0
huh ?


PS4, Line 370: in which
long line


Line 463:   VLOG_QUERY << "BS::PublishFilter(): query_id=" << PrintId(query_id_)
<< " state_idx=" << state_idx_ << " host=" << host_;
long line


http://gerrit.cloudera.org:8080/#/c/6535/4/be/src/runtime/coordinator.cc
File be/src/runtime/coordinator.cc:

Line 340:           // Set the 'pending_count_' to zero to indicate that for a filter with
local-only
long line


PS4, Line 421: //VLOG_QUERY << "backend status: idx=" << backend_state->st
?


PS4, Line 424: VLOG_QUERY << "set new status";
Debugging output ?


PS4, Line 632:  //
Did you mean to comment this line out ?


Line 714:     if (!partition_create_ops.Execute(ExecEnv::GetInstance()->hdfs_op_thread_pool(),
false)) {
long line


Line 976: #if 0
huh ?


http://gerrit.cloudera.org:8080/#/c/6535/4/be/src/runtime/coordinator.h
File be/src/runtime/coordinator.h:

PS4, Line 216: //ExecSummary* exec_summary() { return &exec_summary_; }
Is this not needed ?


PS4, Line 304: lock_
The lock acquisition order is still unclear wrt other locks in this class (e.g. wait_lock_,
filter_lock_) although one may derive it by reading the code. In the long run, we should consider
assigning rank to locks so their acquisition order is implicitly encoded in their ranks.

Also, what's the test coverage for the execution paths which may lead to taking multiple locks
?


PS4, Line 404: before
Mind explaining why it must be called before InitBackendState()  in the comment ?


PS4, Line 444:  
blank space


http://gerrit.cloudera.org:8080/#/c/6535/4/be/src/runtime/descriptors.cc
File be/src/runtime/descriptors.cc:

PS4, Line 500: partition_key_value_ctxs
Isn't the partition expr Literal though ? Does it really need to allocate memory ?


http://gerrit.cloudera.org:8080/#/c/6535/4/be/src/runtime/fragment-instance-state.cc
File be/src/runtime/fragment-instance-state.cc:

Line 77:   //VLOG_QUERY << "~FIS: instance_id=" << PrintId(instance_id()) <<
" this=" << this << " sink=" << sink_;
Debugging output ?


Line 115:   //VLOG_QUERY << "Cancelling fragment instance " << PrintId(instance_id())
<< " this=" << this << " sink=" << sink_;
?


Line 133:   //VLOG_QUERY << "Prepare(): instance_id=" << PrintId(instance_id())
<< " this=" << this << " sink=" << sink_;
?


Line 310:   //VLOG_QUERY << "Close(): instance_id=" << PrintId(instance_id())
<< " this=" << this << " sink=" << sink_;
?


http://gerrit.cloudera.org:8080/#/c/6535/4/be/src/runtime/fragment-instance-state.h
File be/src/runtime/fragment-instance-state.h:

PS4, Line 52: for
long line


PS4, Line 91:  
blank space


http://gerrit.cloudera.org:8080/#/c/6535/4/be/src/runtime/query-state.cc
File be/src/runtime/query-state.cc:

PS4, Line 254: 100
magic constant. Could this be better to be #define or constexpr value ?


PS4, Line 379:   VLOG_QUERY << "PublishFilter: filter_id=" << filter_id <<
" fragment_idx=" << fragment_idx;
long line


http://gerrit.cloudera.org:8080/#/c/6535/4/be/src/runtime/query-state.h
File be/src/runtime/query-state.h:

PS4, Line 172: AtomicInt32 is_cancelled_;
Is there any atomic read-modify-write operation on this flag ? Doesn't seem strictly necessary
to be AtomicInt32.


http://gerrit.cloudera.org:8080/#/c/6535/4/be/src/runtime/runtime-state.cc
File be/src/runtime/runtime-state.cc:

PS4, Line 105: test-pool
What does "test-pool" mean ?


http://gerrit.cloudera.org:8080/#/c/6535/4/be/src/runtime/runtime-state.h
File be/src/runtime/runtime-state.h:

PS4, Line 99: ObjectPoool
typo.


http://gerrit.cloudera.org:8080/#/c/6535/4/be/src/service/impala-hs2-server.cc
File be/src/service/impala-hs2-server.cc:

PS4, Line 194:   // Make sure ClientRequestState::Wait() has completed before fetching rows.
Wait() ensures
             :   // that rows are ready to be fetched (e.g., Wait() opens ClientRequestState::output_exprs_,
long lines


http://gerrit.cloudera.org:8080/#/c/6535/4/be/src/util/uid-util.h
File be/src/util/uid-util.h:

PS4, Line 81: IsValidId
IsValidFid ? It's not entirely clear from the function name what Id is.


http://gerrit.cloudera.org:8080/#/c/6535/4/common/thrift/ImpalaInternalService.thrift
File common/thrift/ImpalaInternalService.thrift:

PS4, Line 427: TExecQueryFInstancesParams 
Can you please add some brief comments on the overview of how these structures relate to each
other ?


PS4, Line 545: V1
What does V1 stand for ? Version 1 ? of what ?


PS4, Line 698: required
Is it optional or required ?


-- 
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: 4
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-HasComments: Yes

Mime
View raw message