Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/13583 )
Change subject: IMPALA-7467: Ported ExecQueryFInstances over to krpc ...................................................................... Patch Set 1: (9 comments) http://gerrit.cloudera.org:8080/#/c/13583/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13583/1//COMMIT_MSG@9 PS1, Line 9: ExecQuerryFInstances ExecQueryFInstances http://gerrit.cloudera.org:8080/#/c/13583/1/be/src/runtime/coordinator-backend-state.cc File be/src/runtime/coordinator-backend-state.cc: http://gerrit.cloudera.org:8080/#/c/13583/1/be/src/runtime/coordinator-backend-state.cc@176 PS1, Line 176: VLOG_QUERY LOG(ERROR) ? http://gerrit.cloudera.org:8080/#/c/13583/1/be/src/runtime/coordinator-backend-state.cc@209 PS1, Line 209: serialized_buf May help to add a remark about the ownership of this buffer. http://gerrit.cloudera.org:8080/#/c/13583/1/be/src/runtime/coordinator-backend-state.cc@214 PS1, Line 214: serialized_buf = nullptr; Why is this necessary ? Also, leave a comment on why we don't need to free the buffer explicitly here. http://gerrit.cloudera.org:8080/#/c/13583/1/be/src/runtime/coordinator-backend-state.cc@218 PS1, Line 218: serialized_buf = nullptr; Same as above. http://gerrit.cloudera.org:8080/#/c/13583/1/be/src/runtime/coordinator-backend-state.cc@233 PS1, Line 233: LOG(DFATAL) << FromKuduStatus(sidecar_status, "Failed to add sidecar").GetDetail(); If we fail to add the sidecar, it doesn't seem to make sense to continue with the RPC as the executor cannot do much without the content in the sidecar, right ? http://gerrit.cloudera.org:8080/#/c/13583/1/be/src/service/control-service.h File be/src/service/control-service.h: http://gerrit.cloudera.org:8080/#/c/13583/1/be/src/service/control-service.h@112 PS1, Line 112: QueryExecMgr* query_exec_mgr_; Is this necessary ? What's wrong with getting it from the singleton ExecEnv() on each RPC call ? It shouldn't be that expensive. http://gerrit.cloudera.org:8080/#/c/13583/1/be/src/service/control-service.cc File be/src/service/control-service.cc: http://gerrit.cloudera.org:8080/#/c/13583/1/be/src/service/control-service.cc@73 PS1, Line 73: query_exec_mgr_ = ExecEnv::GetInstance()->query_exec_mgr(); : DCHECK(query_exec_mgr_ != nullptr); Move to initializer list. http://gerrit.cloudera.org:8080/#/c/13583/1/common/thrift/ImpalaInternalService.thrift File common/thrift/ImpalaInternalService.thrift: http://gerrit.cloudera.org:8080/#/c/13583/1/common/thrift/ImpalaInternalService.thrift@579 PS1, Line 579: struct TExecQueryFInstancesSidecar { Do we plan to clean it up eventually to use protobuf only ? How about leaving a TODO here about converting the followings to protobuf eventually ? -- To view, visit http://gerrit.cloudera.org:8080/13583 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id3f1c6099109bd8e5361188005a7d0e892147570 Gerrit-Change-Number: 13583 Gerrit-PatchSet: 1 Gerrit-Owner: Thomas Marshall <tmarsh...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Comment-Date: Thu, 13 Jun 2019 21:31:54 +0000 Gerrit-HasComments: Yes
