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