Sailesh Mukil has posted comments on this change. Change subject: IMPALA-5744: Add dummy 'use_krpc' flag and create DataStream interface ......................................................................
Patch Set 5: (17 comments) http://gerrit.cloudera.org:8080/#/c/7542/5/be/src/exec/exchange-node.cc File be/src/exec/exchange-node.cc: PS5, Line 80: // TODO: Remove DCHECK when KRPC is supported. : DCHECK(!FLAGS_use_krpc); > won't this get triggered during CreateImpalaServer() anyhow? Not sure wheth Moved all the CHECKs into the Stub implementation, so I removed this. http://gerrit.cloudera.org:8080/#/c/7542/4/be/src/runtime/data-stream-mgr-base.h File be/src/runtime/data-stream-mgr-base.h: Line 54: virtual Status CloseSender(const TUniqueId& fragment_instance_id, PlanNodeId dest_node_id, > nit: long line Done http://gerrit.cloudera.org:8080/#/c/7542/5/be/src/runtime/data-stream-mgr-base.h File be/src/runtime/data-stream-mgr-base.h: PS5, Line 29: MetricGroup > not needed? Nope, removed it. Line 35: /// node. > This is a pure virtual class for the thrift and KRPC implementation. Please Done Line 50: const TRowBatch& thrift_batch, int sender_id) = 0; > nit: wrong indent Done PS5, Line 49: virtual Status AddData(const TUniqueId& fragment_instance_id, PlanNodeId dest_node_id, : const TRowBatch& thrift_batch, int sender_id) = 0; > As discussed offline, since this interface will diverge across the two impl I went with option 1, as that is the less disruptive approach. There is however one shortcoming of doing this, which is that the caller should always cast to the derived class type before calling AddData(). The 2 places this happens is in: ImpalaServer::TransmitData() data-stream-test For ImpalaServer::TransmitData(), I introduced 2 functions in ExecEnv that would return the respective casted type (DataStreamMgr or KrpcDataStreamMgr), and we call AddData() with that pointer. For the data-stream-test, I need to do a special case dynamic_cast since the exec_env_ for tests does not control the DataStreamMgrBase object, therefore I can't reuse the newly introduced functions. Line 54: virtual Status CloseSender(const TUniqueId& fragment_instance_id, PlanNodeId dest_node_id, > nit: long line Done http://gerrit.cloudera.org:8080/#/c/7542/5/be/src/runtime/data-stream-mgr.h File be/src/runtime/data-stream-mgr.h: Line 71: virtual ~DataStreamMgr(); > override? Done http://gerrit.cloudera.org:8080/#/c/7542/5/be/src/runtime/data-stream-recvr-base.h File be/src/runtime/data-stream-recvr-base.h: PS5, Line 22: #include "util/tuple-row-compare.h" > needed? No, I removed it. Line 28: /// Single receiver of an m:n data stream. > Please add a more meaningful description of this base class. Done PS5, Line 39: the > nit: long line Done http://gerrit.cloudera.org:8080/#/c/7542/5/be/src/runtime/data-stream-recvr.h File be/src/runtime/data-stream-recvr.h: Line 67: virtual ~DataStreamRecvr(); > override Done http://gerrit.cloudera.org:8080/#/c/7542/5/be/src/runtime/data-stream-test.cc File be/src/runtime/data-stream-test.cc: Line 611: shared_ptr<DataStreamRecvrBase> stream_recvr = stream_mgr_->CreateRecvr(runtime_state.get(), > long line Done http://gerrit.cloudera.org:8080/#/c/7542/4/be/src/runtime/exec-env.cc File be/src/runtime/exec-env.cc: PS4, Line 167: errorwith > nit: missing space Done http://gerrit.cloudera.org:8080/#/c/7542/5/be/src/runtime/exec-env.cc File be/src/runtime/exec-env.cc: Line 78: DEFINE_bool(use_krpc, false, "Used to indicate whether to use KRPC for the DataStream " > make this DEFINE_bool_hidden until KRPC goes in. Done PS5, Line 167: // TODO: Replace fatal errorwith KRPCDataStreamMgr implementation when it is : // supported. : ABORT_WITH_ERROR("The 'use_krpc' flag is not supported yet. " : "Please disable and restart cluster"); > Please consider adding a stub implementation of DataStreamMgr for KRPC inst Done http://gerrit.cloudera.org:8080/#/c/7542/5/be/src/service/impala-server.cc File be/src/service/impala-server.cc: PS5, Line 1921: // TODO: Remove DCHECK when KRPC is supported. : DCHECK(!FLAGS_use_krpc); > How about we add a stub implementation for KRPC and keep the DCHECK in the Done -- 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: 5 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
