Michael Ho has posted comments on this change. Change subject: IMPALA-5744: Add dummy 'use_krpc' flag and create DataStream interface ......................................................................
Patch Set 6: (11 comments) http://gerrit.cloudera.org:8080/#/c/7542/6/be/src/exec/exchange-node.h File be/src/exec/exchange-node.h: PS6, Line 39: in nit: long line http://gerrit.cloudera.org:8080/#/c/7542/6/be/src/runtime/data-stream-mgr-base.h File be/src/runtime/data-stream-mgr-base.h: PS6, Line 36: impelementations typo. PS6, Line 36: , one for Thrfit and KRPC each for Thrift and KRPC respectively. http://gerrit.cloudera.org:8080/#/c/7542/6/be/src/runtime/data-stream-recvr-base.h File be/src/runtime/data-stream-recvr-base.h: PS6, Line 33: , one for Thrfit and KRPC each for Thrift and KRPC respectively. PS6, Line 33: impelementations implementations PS6, Line 47: / extra / http://gerrit.cloudera.org:8080/#/c/7542/6/be/src/runtime/exec-env.cc File be/src/runtime/exec-env.cc: PS6, Line 79: KRPC "Kudu RPC" for clarity. http://gerrit.cloudera.org:8080/#/c/7542/6/be/src/runtime/exec-env.h File be/src/runtime/exec-env.h: PS6, Line 92: /// We implement these in the .cc file to avoid including header files for the derived : /// classes in this header file; since the implementations require a dynamic cast from : /// the base type DataStreamMgrBase to the respective derived type and the compiler : /// cannot determine the base-derived relationship from just forward declares. IMHO, this doesn't seem to provide much value in explaining it here. Can you please consider removing it or putting it in .cc file if you think it's useful for documentation. Please add extra explanation on why most callers should use stream_mgr() instead of these interfaces. http://gerrit.cloudera.org:8080/#/c/7542/6/be/src/runtime/krpc-data-stream-mgr.h File be/src/runtime/krpc-data-stream-mgr.h: PS6, Line 48: nit: blank space http://gerrit.cloudera.org:8080/#/c/7542/6/be/src/runtime/krpc-data-stream-recvr.cc File be/src/runtime/krpc-data-stream-recvr.cc: PS6, Line 28: Unexpected: A KrpcDataStreamRecvr object should not be " : "created if the 'use_krpc' flag is false. Can be simplified to "Shouldn't reach here unless startup flag 'use_krpc' is true. http://gerrit.cloudera.org:8080/#/c/7542/6/be/src/runtime/krpc-data-stream-recvr.h File be/src/runtime/krpc-data-stream-recvr.h: PS6, Line 30: Krpc nit: KRPC -- To view, visit http://gerrit.cloudera.org:8080/7542 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5d52245154e910529a68f53049520238eca16241 Gerrit-PatchSet: 6 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Sailesh Mukil <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-HasComments: Yes
