Michael Ho has posted comments on this change. Change subject: IMPALA-5744: Add dummy 'use_krpc' flag and create DataStream interface ......................................................................
Patch Set 5: (10 comments) 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 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: Line 35: /// node. This is a pure virtual class for the thrift and KRPC implementation. Please also add a TODO to remove this class and rename all clients once the parallel implementation is removed. Line 50: const TRowBatch& thrift_batch, int sender_id) = 0; nit: wrong indent 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 implementations, we can do either of the following: 1. remove AddData() from the virtual base class and make the client do an explicit type cast to the underlying type. 2. have a common interface and hide the arguments for the two different implementations inside a union. Please consider the options above. I personally think (1) will be least disruptive to the existing code and the eventual removal of the parallel implementation. Line 54: virtual Status CloseSender(const TUniqueId& fragment_instance_id, PlanNodeId dest_node_id, nit: long line 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: Line 28: /// Single receiver of an m:n data stream. Please add a more meaningful description of this base class. PS5, Line 39: the nit: long line 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 http://gerrit.cloudera.org:8080/#/c/7542/5/be/src/runtime/exec-env.cc File be/src/runtime/exec-env.cc: 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 instead of warning here. 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 stub implementation instead. -- 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
