[Impala-ASF-CR] IMPALA-5744: Add dummy 'use krpc' flag and create DataStream interface
Michael Ho has posted comments on this change. Change subject: IMPALA-5744: Add dummy 'use_krpc' flag and create DataStream interface .. Patch Set 8: Code-Review+2 (8 comments) Please also wait for comments from Henry. http://gerrit.cloudera.org:8080/#/c/7542/8//COMMIT_MSG Commit Message: PS8, Line 7: dummy 'use_krpc' flag The flag is not really dummy, right ? We are just using dummy implementation for KRPC for now. Should we just say Add "use_krpc" flag.. Same below. Line 20: DataStream*Base. Using pure virtual base class may result in code duplication but the current approach is to optimize for the eventual removal of the thrift implementations. PS8, Line 27: Since having both the Thrift and KRPC implementations of the : DataStream* classes are only expected to be temporary for now, this : was written and optimized with the end goal of having only the KRPC : versions of the DataStreamMgr/Recvr, at which point we will get rid : of the DataStream*Base classes, the Thrift versions of the classes : and rename KrpcDataStream* to DataStream*. We will also rename all : the references that the clients have to DataStream*Base to DataStream*. May remove or shorten this entire paragraph if you take the one line suggestion above. http://gerrit.cloudera.org:8080/#/c/7542/8/be/src/runtime/data-stream-mgr-base.h File be/src/runtime/data-stream-mgr-base.h: PS8, Line 35: enforces a basic interface defines the basic interface PS8, Line 36: , one each for Thrift and KRPC. for thrift and KRPC respectively. http://gerrit.cloudera.org:8080/#/c/7542/8/be/src/runtime/data-stream-recvr-base.h File be/src/runtime/data-stream-recvr-base.h: PS8, Line 32: enforces a basic interface defines the basic interface PS8, Line 33: , one each for Thrift and KRPC for thrift and KRPC respectively. http://gerrit.cloudera.org:8080/#/c/7542/8/be/src/runtime/exec-env.h File be/src/runtime/exec-env.h: PS8, Line 92: Clients of the implementations of DataStreamMgrBase should not use these functions Clients of DataStreamMgrBase should use stream_mgr() unless ... -- 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: 8 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Sailesh MukilGerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5744: Add dummy 'use krpc' flag and create DataStream interface
Sailesh Mukil has uploaded a new patch set (#8). Change subject: IMPALA-5744: Add dummy 'use_krpc' flag and create DataStream interface .. IMPALA-5744: Add dummy 'use_krpc' flag and create DataStream interface This patch introduces a dummy 'use_krpc' flag and creates an abstract interface for the DataStreamRecvr/Mgr. The 'use_krpc' flag defaults to 'false'. Cluster startup will abort with an error if the flag is switched to 'true'. The DataStreamSender implements the same virtual interface as the DataSink, so a pure virtual class for the DataStreamSender would essentially be an empty class. Therefore, it is not implemented. The new interfaces are pure virtual base classes and are named DataStream*Base. Stubs for the Krpc implementations are also introduced and are named KrpcDataStream*. They currently only abort with a fatal error if they are used. Their actual implementations will be filled in a later patch. Since having both the Thrift and KRPC implementations of the DataStream* classes are only expected to be temporary for now, this was written and optimized with the end goal of having only the KRPC versions of the DataStreamMgr/Recvr, at which point we will get rid of the DataStream*Base classes, the Thrift versions of the classes and rename KrpcDataStream* to DataStream*. We will also rename all the references that the clients have to DataStream*Base to DataStream*. Also did some spurious includes cleanup. Change-Id: I5d52245154e910529a68f53049520238eca16241 --- M be/src/exec/data-sink.cc M be/src/exec/exchange-node.cc M be/src/exec/exchange-node.h M be/src/runtime/CMakeLists.txt A be/src/runtime/data-stream-mgr-base.h M be/src/runtime/data-stream-mgr.cc M be/src/runtime/data-stream-mgr.h A be/src/runtime/data-stream-recvr-base.h M be/src/runtime/data-stream-recvr.h M be/src/runtime/data-stream-test.cc M be/src/runtime/exec-env.cc M be/src/runtime/exec-env.h M be/src/runtime/fragment-instance-state.cc A be/src/runtime/krpc-data-stream-mgr.cc A be/src/runtime/krpc-data-stream-mgr.h A be/src/runtime/krpc-data-stream-recvr.cc A be/src/runtime/krpc-data-stream-recvr.h M be/src/runtime/runtime-state.cc M be/src/runtime/runtime-state.h M be/src/service/impala-server.cc 20 files changed, 442 insertions(+), 39 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/42/7542/8 -- To view, visit http://gerrit.cloudera.org:8080/7542 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I5d52245154e910529a68f53049520238eca16241 Gerrit-PatchSet: 8 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Sailesh MukilGerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil
[Impala-ASF-CR] IMPALA-5744: Add dummy 'use krpc' flag and create DataStream interface
Sailesh Mukil has posted comments on this change. Change subject: IMPALA-5744: Add dummy 'use_krpc' flag and create DataStream interface .. Patch Set 7: (10 comments) http://gerrit.cloudera.org:8080/#/c/7542/7/be/src/runtime/data-stream-mgr-base.h File be/src/runtime/data-stream-mgr-base.h: PS7, Line 36: Thrfit and KRPC : /// respectively. Remove this when we evenually get rid of the Thrift implementation and : /// replace client references to this class with the KRPC's version of the : /// DataStreamMgrBase. > same comments and typos as elsewhere. Done http://gerrit.cloudera.org:8080/#/c/7542/7/be/src/runtime/data-stream-mgr.h File be/src/runtime/data-stream-mgr.h: PS7, Line 22: #include "runtime/data-stream-mgr-base.h" > Move to line 33, with the other Impala includes. Done http://gerrit.cloudera.org:8080/#/c/7542/7/be/src/runtime/data-stream-recvr-base.h File be/src/runtime/data-stream-recvr-base.h: PS7, Line 33: Thrfit > typo Done PS7, Line 34: respectively > remove (there's nothing in this sentence for them to be respective to). Done PS7, Line 34: when we evenually get rid of the Thrift implementation and : /// replace client references to this class with the KRPC's version of the : /// DataStreamRecvrBase. > "in favor of the KRPC implementation when possible". Done http://gerrit.cloudera.org:8080/#/c/7542/7/be/src/runtime/data-stream-recvr.h File be/src/runtime/data-stream-recvr.h: PS7, Line 42: /// Single receiver of an m:n data stream. > This should say something about the fact that it's the thrift implementatio Done http://gerrit.cloudera.org:8080/#/c/7542/7/be/src/runtime/data-stream-test.cc File be/src/runtime/data-stream-test.cc: PS7, Line 460: ImpalaTestBackend > Is it hard to convert ImpalaTestBackend to accept a DataStreamMgrBase ? That would mean I have to do the dynamic cast of the 'mgr_' object in L100 from DataStreamMgrBase to DataStreamMgr, which I thought was a little more ugly than this test client class explicitly using a DataStreamMgr. http://gerrit.cloudera.org:8080/#/c/7542/7/be/src/runtime/exec-env.h File be/src/runtime/exec-env.h: PS7, Line 93: enforced by > 'part of' ? Done PS7, Line 94: (This is due to the stream mgrs using different AddData() signatures) > Would remove this. Done http://gerrit.cloudera.org:8080/#/c/7542/7/be/src/runtime/krpc-data-stream-recvr.cc File be/src/runtime/krpc-data-stream-recvr.cc: Line 52: } > return Status::OK()? Otherwise clang-tidy might complain. Here and elsewher 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: 7 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Sailesh MukilGerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5744: Add dummy 'use krpc' flag and create DataStream interface
Henry Robinson has posted comments on this change. Change subject: IMPALA-5744: Add dummy 'use_krpc' flag and create DataStream interface .. Patch Set 7: (10 comments) Looks pretty close to me. http://gerrit.cloudera.org:8080/#/c/7542/7/be/src/runtime/data-stream-mgr-base.h File be/src/runtime/data-stream-mgr-base.h: PS7, Line 36: Thrfit and KRPC : /// respectively. Remove this when we evenually get rid of the Thrift implementation and : /// replace client references to this class with the KRPC's version of the : /// DataStreamMgrBase. same comments and typos as elsewhere. http://gerrit.cloudera.org:8080/#/c/7542/7/be/src/runtime/data-stream-mgr.h File be/src/runtime/data-stream-mgr.h: PS7, Line 22: #include "runtime/data-stream-mgr-base.h" Move to line 33, with the other Impala includes. http://gerrit.cloudera.org:8080/#/c/7542/7/be/src/runtime/data-stream-recvr-base.h File be/src/runtime/data-stream-recvr-base.h: PS7, Line 33: Thrfit typo PS7, Line 34: respectively remove (there's nothing in this sentence for them to be respective to). PS7, Line 34: when we evenually get rid of the Thrift implementation and : /// replace client references to this class with the KRPC's version of the : /// DataStreamRecvrBase. "in favor of the KRPC implementation when possible". http://gerrit.cloudera.org:8080/#/c/7542/7/be/src/runtime/data-stream-recvr.h File be/src/runtime/data-stream-recvr.h: PS7, Line 42: /// Single receiver of an m:n data stream. This should say something about the fact that it's the thrift implementation. http://gerrit.cloudera.org:8080/#/c/7542/7/be/src/runtime/data-stream-test.cc File be/src/runtime/data-stream-test.cc: PS7, Line 460: ImpalaTestBackend Is it hard to convert ImpalaTestBackend to accept a DataStreamMgrBase ? http://gerrit.cloudera.org:8080/#/c/7542/7/be/src/runtime/exec-env.h File be/src/runtime/exec-env.h: PS7, Line 93: enforced by 'part of' ? PS7, Line 94: (This is due to the stream mgrs using different AddData() signatures) Would remove this. http://gerrit.cloudera.org:8080/#/c/7542/7/be/src/runtime/krpc-data-stream-recvr.cc File be/src/runtime/krpc-data-stream-recvr.cc: Line 52: } return Status::OK()? Otherwise clang-tidy might complain. Here and elsewhere. -- 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: 7 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Sailesh MukilGerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5744: Add dummy 'use krpc' flag and create DataStream interface
Sailesh Mukil has posted comments on this change. Change subject: IMPALA-5744: Add dummy 'use_krpc' flag and create DataStream interface .. Patch Set 7: (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: > nit: long line Done 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: one each for Thrfit and KRPC > for Thrift and KRPC respectively. Done PS6, Line 36: implementations > typo. Done 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 each for Thrfit and KRPC > for Thrift and KRPC respectively. Done PS6, Line 33: implementations > implementations Done PS6, Line 47: > extra / Done http://gerrit.cloudera.org:8080/#/c/7542/6/be/src/runtime/exec-env.cc File be/src/runtime/exec-env.cc: PS6, Line 79: Kudu > "Kudu RPC" for clarity. Done http://gerrit.cloudera.org:8080/#/c/7542/6/be/src/runtime/exec-env.h File be/src/runtime/exec-env.h: PS6, Line 92: /// Clients of the implementations of DataStreamMgrBase should not use these functions : /// unless they need to access members that are not enforced by the DataStreamMgrBase : /// interface. (This is due to the stream mgrs using different AddData() signatures) : DataStreamMgr* ThriftStreamMgr(); > IMHO, this doesn't seem to provide much value in explaining it here. Can yo I added it because we have a habit of including getters in the header file, and this is an exception. But I too agree that it's not very necessary to explain, so I removed it. Added the extra explanation too. 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 Done 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: Shouldn't reach here unless startup flag 'use_krpc' " : "is true."; > Can be simplified to "Shouldn't reach here unless startup flag 'use_krpc' i Done 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 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: 7 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Sailesh MukilGerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5744: Add dummy 'use krpc' flag and create DataStream interface
Sailesh Mukil has uploaded a new patch set (#7). Change subject: IMPALA-5744: Add dummy 'use_krpc' flag and create DataStream interface .. IMPALA-5744: Add dummy 'use_krpc' flag and create DataStream interface This patch introduces a dummy 'use_krpc' flag and creates an abstract interface for the DataStreamRecvr/Mgr. The 'use_krpc' flag defaults to 'false'. Cluster startup will abort with an error if the flag is switched to 'true'. The DataStreamSender implements the same virtual interface as the DataSink, so a pure virtual class for the DataStreamSender would essentially be an empty class. Therefore, it is not implemented. The new interfaces are pure virtual base classes and are named DataStream*Base. Stubs for the Krpc implementations are also introduced and are named KrpcDataStream*. They currently only abort with a fatal error if they are used. Their actual implementations will be filled in a later patch. Since having both the Thrift and KRPC implementations of the DataStream* classes are only expected to be temporary for now, this was written and optimized with the end goal of having only the KRPC versions of the DataStreamMgr/Recvr, at which point we will get rid of the DataStream*Base classes, the Thrift versions of the classes and rename KrpcDataStream* to DataStream*. We will also rename all the references that the clients have to DataStream*Base to DataStream*. Also did some spurious includes cleanup. Change-Id: I5d52245154e910529a68f53049520238eca16241 --- M be/src/exec/data-sink.cc M be/src/exec/exchange-node.cc M be/src/exec/exchange-node.h M be/src/runtime/CMakeLists.txt A be/src/runtime/data-stream-mgr-base.h M be/src/runtime/data-stream-mgr.cc M be/src/runtime/data-stream-mgr.h A be/src/runtime/data-stream-recvr-base.h M be/src/runtime/data-stream-recvr.h M be/src/runtime/data-stream-test.cc M be/src/runtime/exec-env.cc M be/src/runtime/exec-env.h M be/src/runtime/fragment-instance-state.cc A be/src/runtime/krpc-data-stream-mgr.cc A be/src/runtime/krpc-data-stream-mgr.h A be/src/runtime/krpc-data-stream-recvr.cc A be/src/runtime/krpc-data-stream-recvr.h M be/src/runtime/runtime-state.cc M be/src/runtime/runtime-state.h M be/src/service/impala-server.cc 20 files changed, 442 insertions(+), 39 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/42/7542/7 -- To view, visit http://gerrit.cloudera.org:8080/7542 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I5d52245154e910529a68f53049520238eca16241 Gerrit-PatchSet: 7 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Sailesh MukilGerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil
[Impala-ASF-CR] IMPALA-5744: Add dummy 'use krpc' flag and create DataStream interface
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 MukilGerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5744: Add dummy 'use krpc' flag and create DataStream interface
Sailesh Mukil has uploaded a new patch set (#6). Change subject: IMPALA-5744: Add dummy 'use_krpc' flag and create DataStream interface .. IMPALA-5744: Add dummy 'use_krpc' flag and create DataStream interface This patch introduces a dummy 'use_krpc' flag and creates an abstract interface for the DataStreamRecvr/Mgr. The 'use_krpc' flag defaults to 'false'. Cluster startup will abort with an error if the flag is switched to 'true'. The DataStreamSender implements the same virtual interface as the DataSink, so a pure virtual class for the DataStreamSender would essentially be an empty class. Therefore, it is not implemented. The new interfaces are pure virtual base classes and are named DataStream*Base. Stubs for the Krpc implementations are also introduced and are named KrpcDataStream*. They currently only abort with a fatal error if they are used. Their actual implementations will be filled in a later patch. Since having both the Thrift and KRPC implementations of the DataStream* classes are only expected to be temporary for now, this was written and optimized with the end goal of having only the KRPC versions of the DataStreamMgr/Recvr, at which point we will get rid of the DataStream*Base classes, the Thrift versions of the classes and rename KrpcDataStream* to DataStream*. We will also rename all the references that the clients have to DataStream*Base to DataStream*. Change-Id: I5d52245154e910529a68f53049520238eca16241 --- M be/src/exec/data-sink.cc M be/src/exec/exchange-node.cc M be/src/exec/exchange-node.h M be/src/runtime/CMakeLists.txt A be/src/runtime/data-stream-mgr-base.h M be/src/runtime/data-stream-mgr.cc M be/src/runtime/data-stream-mgr.h A be/src/runtime/data-stream-recvr-base.h M be/src/runtime/data-stream-recvr.h M be/src/runtime/data-stream-test.cc M be/src/runtime/exec-env.cc M be/src/runtime/exec-env.h M be/src/runtime/fragment-instance-state.cc A be/src/runtime/krpc-data-stream-mgr.cc A be/src/runtime/krpc-data-stream-mgr.h A be/src/runtime/krpc-data-stream-recvr.cc A be/src/runtime/krpc-data-stream-recvr.h M be/src/runtime/runtime-state.cc M be/src/runtime/runtime-state.h M be/src/service/impala-server.cc 20 files changed, 440 insertions(+), 38 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/42/7542/6 -- To view, visit http://gerrit.cloudera.org:8080/7542 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I5d52245154e910529a68f53049520238eca16241 Gerrit-PatchSet: 6 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Sailesh MukilGerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil
[Impala-ASF-CR] IMPALA-5744: Add dummy 'use krpc' flag and create DataStream interface
Henry Robinson has posted comments on this change. Change subject: IMPALA-5744: Add dummy 'use_krpc' flag and create DataStream interface .. Patch Set 5: (7 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 whether it's worth repeating this check. 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? 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? 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? 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 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 stream_recvr = stream_mgr_->CreateRecvr(runtime_state.get(), long line 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. -- 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 MukilGerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5744: Add dummy 'use krpc' flag and create DataStream interface
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 MukilGerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5744: Add dummy 'use krpc' flag and create DataStream interface
Henry Robinson has posted comments on this change. Change subject: IMPALA-5744: Add dummy 'use_krpc' flag and create DataStream interface .. Patch Set 4: (2 comments) I think you need to replace the types of the objects with their base classes where appropriate. For example, ExecEnv::data_stream_mgr_ needs to be a DataStreamMgrBase. http://gerrit.cloudera.org:8080/#/c/7542/4/be/src/runtime/data-stream-mgr.h File be/src/runtime/data-stream-mgr.h: Line 71: ~DataStreamMgr(); not virtual? Line 82: bool is_merging); you'll need to mark the inherited methods as 'override', or clang-tidy will complain. -- 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: 4 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Sailesh MukilGerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5744: Add dummy 'use krpc' flag and create DataStream interface
Sailesh Mukil has uploaded a new patch set (#4). Change subject: IMPALA-5744: Add dummy 'use_krpc' flag and create DataStream interface .. IMPALA-5744: Add dummy 'use_krpc' flag and create DataStream interface This patch introduces a dummy 'use_krpc' flag and creates an abstract interface for the DataStreamRecvr/Mgr. The DataStreamSender implements the same virtual interface as the DataSink, so a pure virtual class for the DataStreamSender would essentially be an empty class. Therefore, it is not implemented. The new interfaces are pure virtual base classes and are named DataStream*Base. The future KRPC patches will also implement the DataStream*Base interfaces. The 'use_krpc' flag defaults to 'false'. Cluster startup will abort with an error if the flag is switched to 'true'. Change-Id: I5d52245154e910529a68f53049520238eca16241 --- M be/src/exec/data-sink.cc M be/src/exec/exchange-node.cc M be/src/runtime/CMakeLists.txt A be/src/runtime/data-stream-mgr-base.h M be/src/runtime/data-stream-mgr.cc M be/src/runtime/data-stream-mgr.h A be/src/runtime/data-stream-recvr-base.h M be/src/runtime/data-stream-recvr.h M be/src/runtime/exec-env.cc M be/src/runtime/fragment-instance-state.cc M be/src/service/impala-server.cc 11 files changed, 156 insertions(+), 5 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/42/7542/4 -- To view, visit http://gerrit.cloudera.org:8080/7542 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I5d52245154e910529a68f53049520238eca16241 Gerrit-PatchSet: 4 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Sailesh MukilGerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil
[Impala-ASF-CR] IMPALA-5744: Add dummy 'use krpc' flag and create DataStream interface
Michael Ho has posted comments on this change. Change subject: IMPALA-5744: Add dummy 'use_krpc' flag and create DataStream interface .. Patch Set 3: Re-reading the commit message, the renaming seems inevitable with the abstract class approach. -- 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: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Sailesh MukilGerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5744: Add dummy 'use krpc' flag and create DataStream interface
Michael Ho has posted comments on this change. Change subject: IMPALA-5744: Add dummy 'use_krpc' flag and create DataStream interface .. Patch Set 1: FWIW, I was actually expecting that the renaming would happen later. This makes the change easier to review (as one can tell if anything is actually changed in DSS code). I understand this is optimizing for the eventual outcome which is to remove thrift-data* but we may still have the code with us for a couple of releases (as backup options at least in the initial release of KRPC). What do you think ? -- 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: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Sailesh MukilGerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5744: Add dummy 'use krpc' flag and create DataStream interface
Sailesh Mukil has posted comments on this change. Change subject: IMPALA-5744: Add dummy 'use_krpc' flag and create DataStream interface .. Patch Set 3: > (1 comment) > > Quick question before I get stuck in - did you evaluate the > complexity of building this class hierarchy vs having the users of > the datastream interfaces keeping a pointer to both possible > implementations, and using FLAGS_use_krpc to decide which one to > call into? > > It would be a bit tedious in, e.g. exchange-node.cc - but it would > be self-contained, and it would be easy to undo the changes when we > standardize on one solution. With the class hierarchy approach, I'm > a bit concerned about the amount of code we introduce that will > ultimately be removed. That's a good question. Fortunately, the short answer is that it's easy to undo the changes with this class hierarchy approach. If you have a look at the files, most of the changes are whole file diffs. The users of the interface have almost no code change except for specifying which DataStream* class to use (Thrift or KRPC). When we plan to undo these changes, we can follow these steps: - Completely remove thrift-data-stream-* files. - De-virtualize the DataStream* abstract classes functions. - Move the KRPCDataStream* specific members and functions into the DataStream* class. - Remove the 'use_krpc' flag and the places it's being used in. - Create DataStream* objects always instead of choosing between ThriftDataStream* or KRPCDataStream*. If we choose to not have the abstract classes and just have the users have 2 pointers, one to each implementation, the steps will remain the same except for the 'de-virtualize DataStream*' and 'move KRPCDataStream* into DataStream*' steps, which IMO is not saving us a lot of work. Moreover, the abstract class way seems cleaner. So, I would opt to keep it this way but I'm open to suggestions as well. -- 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: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Sailesh MukilGerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5744: Add dummy 'use krpc' flag and create DataStream interface
Henry Robinson has posted comments on this change. Change subject: IMPALA-5744: Add dummy 'use_krpc' flag and create DataStream interface .. Patch Set 1: (1 comment) Quick question before I get stuck in - did you evaluate the complexity of building this class hierarchy vs having the users of the datastream interfaces keeping a pointer to both possible implementations, and using FLAGS_use_krpc to decide which one to call into? It would be a bit tedious in, e.g. exchange-node.cc - but it would be self-contained, and it would be easy to undo the changes when we standardize on one solution. With the class hierarchy approach, I'm a bit concerned about the amount of code we introduce that will ultimately be removed. http://gerrit.cloudera.org:8080/#/c/7542/1/be/src/runtime/sender-queue-base.h File be/src/runtime/sender-queue-base.h: PS1, Line 28: #include "common/names.h" this shouldn't be included in a .h file -- 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: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Sailesh MukilGerrit-Reviewer: Henry Robinson Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5744: Add dummy 'use krpc' flag and create DataStream interface
Sailesh Mukil has uploaded a new patch set (#3). Change subject: IMPALA-5744: Add dummy 'use_krpc' flag and create DataStream interface .. IMPALA-5744: Add dummy 'use_krpc' flag and create DataStream interface This patch introduces a dummy 'use_krpc' flag and creates an abstract interface for the DataStreamSender/Recvr/Mgr. The current DataStreamSender/Recvr/Mgr are renamed to ThriftDataStream* and inherit from the new abstract DataStream* interface. The DataStreamRecvr::SenderQueue is not a nested class anymore and is converted to SenderQueueBase which is an abstract class itself. The ThriftDataStreamRecvr::SenderQueue implements the interface exposed by SenderQueueBase. The future KRPC patches will also implement the DataStream* interfaces and the SenderQueueBase interface. The 'use_krpc' flag defaults to 'false'. Cluster startup will abort with an error if the flag is switched to 'true'. DataStreamTest is renamed to ThriftDataStreamTest. Change-Id: I5d52245154e910529a68f53049520238eca16241 --- M be/src/exec/data-sink.cc M be/src/exec/exchange-node.cc M be/src/runtime/CMakeLists.txt M be/src/runtime/data-stream-mgr.cc M be/src/runtime/data-stream-mgr.h M be/src/runtime/data-stream-recvr.cc M be/src/runtime/data-stream-recvr.h M be/src/runtime/data-stream-sender.cc M be/src/runtime/data-stream-sender.h M be/src/runtime/exec-env.cc M be/src/runtime/fragment-instance-state.cc M be/src/runtime/runtime-state.cc A be/src/runtime/sender-queue-base.h A be/src/runtime/thrift-data-stream-mgr.cc A be/src/runtime/thrift-data-stream-mgr.h A be/src/runtime/thrift-data-stream-recvr.cc A be/src/runtime/thrift-data-stream-recvr.h A be/src/runtime/thrift-data-stream-sender.cc A be/src/runtime/thrift-data-stream-sender.h R be/src/runtime/thrift-data-stream-test.cc M be/src/service/impala-server.cc 21 files changed, 1,503 insertions(+), 1,095 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/42/7542/3 -- To view, visit http://gerrit.cloudera.org:8080/7542 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I5d52245154e910529a68f53049520238eca16241 Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Sailesh Mukil
[Impala-ASF-CR] IMPALA-5744: Add dummy 'use krpc' flag and create DataStream interface
Sailesh Mukil has uploaded a new patch set (#2). Change subject: IMPALA-5744: Add dummy 'use_krpc' flag and create DataStream interface .. IMPALA-5744: Add dummy 'use_krpc' flag and create DataStream interface This patch introduces a dummy 'use_krpc' flag and creates an abstract interface for the DataStreamSender/Recvr/Mgr. The current DataStreamSender/Recvr/Mgr are renamed to ThriftDataStream* and inherit from the new abstract DataStream* interface. The DataStreamRecvr::SenderQueue is not a nested class anymore and is converted to SenderQueueBase which is an abstract class itself. The ThriftDataStreamRecvr::SenderQueue implements the interface exposed by SenderQueueBase. The future KRPC patches will also implement the DataStream* interfaces and the SenderQueueBase interface. The 'use_krpc' flag defaults to 'false'. Cluster startup will abort with an error if the flag is switched to 'true'. DataStreamTest is renamed to ThriftDataStreamTest. Change-Id: I5d52245154e910529a68f53049520238eca16241 --- M be/src/exec/data-sink.cc M be/src/exec/exchange-node.cc M be/src/runtime/CMakeLists.txt M be/src/runtime/data-stream-mgr.cc M be/src/runtime/data-stream-mgr.h M be/src/runtime/data-stream-recvr.cc M be/src/runtime/data-stream-recvr.h M be/src/runtime/data-stream-sender.cc M be/src/runtime/data-stream-sender.h M be/src/runtime/exec-env.cc M be/src/runtime/fragment-instance-state.cc M be/src/runtime/runtime-state.cc A be/src/runtime/sender-queue-base.h A be/src/runtime/thrift-data-stream-mgr.cc A be/src/runtime/thrift-data-stream-mgr.h A be/src/runtime/thrift-data-stream-recvr.cc A be/src/runtime/thrift-data-stream-recvr.h A be/src/runtime/thrift-data-stream-sender.cc A be/src/runtime/thrift-data-stream-sender.h R be/src/runtime/thrift-data-stream-test.cc M be/src/service/impala-server.cc 21 files changed, 1,502 insertions(+), 1,094 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/42/7542/2 -- To view, visit http://gerrit.cloudera.org:8080/7542 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I5d52245154e910529a68f53049520238eca16241 Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Sailesh Mukil
[Impala-ASF-CR] IMPALA-5744: Add dummy 'use krpc' flag and create DataStream interface
Sailesh Mukil has uploaded a new change for review. http://gerrit.cloudera.org:8080/7542 Change subject: IMPALA-5744: Add dummy 'use_krpc' flag and create DataStream interface .. IMPALA-5744: Add dummy 'use_krpc' flag and create DataStream interface This patch introduces a dummy 'use_krpc' flag and creates an abstract interface for the DataStreamSender/Recvr/Mgr. The current DataStreamSender/Recvr/Mgr are renamed to ThriftDataStream* and inherit from the new abstract DataStream* interface. The DataStreamRecvr::SenderQueue is not a nested class anymore and is converted to SenderQueueBase which is an abstract class itself. The ThriftDataStreamRecvr::SenderQueue implements the interface exposed by SenderQueueBase. The future KRPC patches will also implement the DataStream* interfaces and the SenderQueueBase interface. The 'use_krpc' flag defaults to 'false'. Cluster startup will abort with an error if the flag is switched to 'true'. Change-Id: I5d52245154e910529a68f53049520238eca16241 --- M be/src/exec/data-sink.cc M be/src/exec/exchange-node.cc M be/src/runtime/CMakeLists.txt M be/src/runtime/data-stream-mgr.cc M be/src/runtime/data-stream-mgr.h M be/src/runtime/data-stream-recvr.cc M be/src/runtime/data-stream-recvr.h M be/src/runtime/data-stream-sender.cc M be/src/runtime/data-stream-sender.h M be/src/runtime/data-stream-test.cc M be/src/runtime/exec-env.cc M be/src/runtime/fragment-instance-state.cc M be/src/runtime/runtime-state.cc A be/src/runtime/sender-queue-base.h A be/src/runtime/thrift-data-stream-mgr.cc A be/src/runtime/thrift-data-stream-mgr.h A be/src/runtime/thrift-data-stream-recvr.cc A be/src/runtime/thrift-data-stream-recvr.h A be/src/runtime/thrift-data-stream-sender.cc A be/src/runtime/thrift-data-stream-sender.h M be/src/service/impala-server.cc 21 files changed, 1,489 insertions(+), 1,081 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/42/7542/1 -- To view, visit http://gerrit.cloudera.org:8080/7542 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I5d52245154e910529a68f53049520238eca16241 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Sailesh Mukil