Marcel Kornacker has posted comments on this change. Change subject: IMPALA-2550: Switch to per-query exec rpc ......................................................................
Patch Set 4: (19 comments) http://gerrit.cloudera.org:8080/#/c/6535/4/be/src/runtime/coordinator-backend-state.cc File be/src/runtime/coordinator-backend-state.cc: PS4, Line 208: #if 0 > huh ? removed http://gerrit.cloudera.org:8080/#/c/6535/4/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: PS4, Line 424: VLOG_QUERY << "set new status"; > Debugging output ? yes, i'll clean all that up before it goes in. PS4, Line 632: // > Did you mean to comment this line out ? Done Line 714: if (!partition_create_ops.Execute(ExecEnv::GetInstance()->hdfs_op_thread_pool(), false)) { > long line Done Line 976: #if 0 > huh ? i think henry put there, i'd like to hear from someone whether we need to keep it. 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 ? Done PS4, Line 304: lock_ > The lock acquisition order is still unclear wrt other locks in this class ( agreed, this needs to be further simplified and clarified, but it's out of scope for this already very large change. tests: existing stress PS4, Line 404: before > Mind explaining why it must be called before InitBackendState() in the com Done PS4, Line 444: > blank space Done 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 m it doesn't, but i didn't want to mess with exprctx::prepare too much, it uses tracker to create a new pool. i'll leave a todo to clean this up. 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 91: > blank space Done 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 ? Done 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 ? nothing specific. that's the name we've been using for tests. 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. Done 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 Done 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. Done 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 structu Done PS4, Line 545: V1 > What does V1 stand for ? Version 1 ? of what ? ImpalaInternalServiceVersion PS4, Line 698: required > Is it optional or required ? in rpc structs, all parameters other than the protocol version need to be optional, otherwise you can't change them later. but for a specific version, a field can still be 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
