impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Sailesh Mukil (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5744: Add dummy 'use krpc' flag and create DataStream interface
Date Thu, 03 Aug 2017 01:36:54 GMT
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 <sailesh@cloudera.com>
Gerrit-Reviewer: Henry Robinson <henry@cloudera.com>
Gerrit-Reviewer: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sailesh@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message