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-4856: Port data stream service to KRPC
Date Fri, 16 Jun 2017 22:13:34 GMT
Sailesh Mukil has posted comments on this change.

Change subject: IMPALA-4856: Port data stream service to KRPC
......................................................................


Patch Set 3:

(17 comments)

http://gerrit.cloudera.org:8080/#/c/7103/3/be/src/rpc/rpc.h
File be/src/rpc/rpc.h:

PS3, Line 147: func
Having the names 'func', 'cb' and 'cb_wrapper' all close by each other makes some of this
slightly more complicated to read.

I would opt for renaming 'func' to 'rpc_method' or something, so that it's crystal clear that
it may not be a callback itself.


PS3, Line 153: std::move(params_)
Does this move mean that the params_ member is invalid after this call? If so, it would be
good to add a comment where it's declared.


PS3, Line 327: cb
Maybe name this 'completion_cb'.


http://gerrit.cloudera.org:8080/#/c/7103/3/be/src/runtime/coordinator-backend-state.cc
File be/src/runtime/coordinator-backend-state.cc:

PS3, Line 389: Copying proto_filter here ensures that its lifetime will last at least until
this
             :   // callback completes.
             :   auto cb = [proto_filter]
Why wouldn't a move capture ensure the same thing?


http://gerrit.cloudera.org:8080/#/c/7103/3/be/src/runtime/coordinator.cc
File be/src/runtime/coordinator.cc:

PS3, Line 1207:       // Do an explicit copy of the directory: 'params' may have come from
an RPC so we
              :       // can't assume ownership of its directory.
              :       bloom_filter_ =
              :           make_unique<ProtoBloomFilter>(params.header, params.directory);
I would add this to the commit message. This means we would take double the memory cost every
time we merge filters right?


http://gerrit.cloudera.org:8080/#/c/7103/3/be/src/runtime/data-stream-recvr.cc
File be/src/runtime/data-stream-recvr.cc:

PS3, Line 280: blocked_senders_.front()
Is this a right way to dispose a unique_ptr?


http://gerrit.cloudera.org:8080/#/c/7103/3/be/src/runtime/data-stream-sender.cc
File be/src/runtime/data-stream-sender.cc:

PS3, Line 58: DECLARE_int32(datastream_sender_timeout_ms);
Not used.


PS3, Line 60: DECLARE_int32(state_store_subscriber_port);
Not used.


PS3, Line 133: AtomicInt64 num_data_bytes_sent_
No one really calls GetNumDataBytesSent() (except from our BE test). So, I'm not sure we're
gaining anything by making this atomic.
Not a big deal, but it might be cheaper to leave it as an int64. Your call though.


PS3, Line 148: self_
A reader of this code might not immediately understand why this class needs to always have
a reference to itself. It would be good to explicitly mention this member name in the header
where the explanation is given.


PS3, Line 170: self_ = shared_from_this();
Why is this set in Init()? Wouldn't it ideally be set it in the constructor?


PS3, Line 175: auto proto_batch
Just want to make sure that this will increase the shared_ptr refcount? It should because
it will make an underlying copy of the pointer, but I just want to make sure.


PS3, Line 203: cb
Prefer a more descriptive name "rpc_completion_cb" or something similar.


Line 336:   batch_.reset();
DCHECK(self_.get())


http://gerrit.cloudera.org:8080/#/c/7103/3/be/src/runtime/row-batch.cc
File be/src/runtime/row-batch.cc:

Line 216:       output_batch->header.set_compression_type(THdfsCompression::LZ4);
Do you think it would be good to add a comment why we don't free the 'tuple_data' buffer here?
Presumably so we can reuse the memory when the RowBatch is recycled?


http://gerrit.cloudera.org:8080/#/c/7103/3/be/src/runtime/row-batch.h
File be/src/runtime/row-batch.h:

PS3, Line 73: == THdfsCompression::LZ4
!= THdfsCompression::NONE

Functionally same, more readable.


PS3, Line 441: FlushMode flush_ = FlushMode::NO_FLUSH_RESOURCES
Why not initialize this and the other args below with the default values in the constructor
member initialization list?


-- 
To view, visit http://gerrit.cloudera.org:8080/7103
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia66704be7a0a8162bb85556d07b583ec756c584b
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <henry@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