Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/13583 )
Change subject: IMPALA-7467: Ported ExecQueryFInstances over to krpc ...................................................................... Patch Set 2: (8 comments) 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: LOG(ERROR) > LOG(ERROR) ? Done 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. Done http://gerrit.cloudera.org:8080/#/c/13583/1/be/src/runtime/coordinator-backend-state.cc@214 PS1, Line 214: SetExecError(serialize_st > Why is this necessary ? Done http://gerrit.cloudera.org:8080/#/c/13583/1/be/src/runtime/coordinator-backend-state.cc@218 PS1, Line 218: Status::Expected("Ser > Same as above. Done http://gerrit.cloudera.org:8080/#/c/13583/1/be/src/runtime/coordinator-backend-state.cc@233 PS1, Line 233: quest.set_sidecar_idx(sidecar_idx); > If we fail to add the sidecar, it doesn't seem to make sense to continue wi Done 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: /// call within 'rpc_context'. > Is this necessary ? What's wrong with getting it from the singleton ExecEnv Done 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: } : > Move to initializer list. Done 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: // TODO: convert this fully to protobuf. > Do we plan to clean it up eventually to use protobuf only ? How about leavi Yeah, but that's a potentially very large change (eg. requires rewriting all of the toThrift()s for Exprs/PlanNodes in the FE to toProtobuf()s), so probably makes sense to do it in pieces, if possible -- 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: 2 Gerrit-Owner: Thomas Marshall <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Thomas Marshall <[email protected]> Gerrit-Comment-Date: Tue, 18 Jun 2019 03:21:53 +0000 Gerrit-HasComments: Yes
