impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Sailesh Mukil (Code Review)" <>
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:

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'.
File be/src/runtime/

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

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,;
I would add this to the commit message. This means we would take double the memory cost every
time we merge filters right?
File be/src/runtime/

PS3, Line 280: blocked_senders_.front()
Is this a right way to dispose a unique_ptr?
File be/src/runtime/

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();
File be/src/runtime/

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?
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
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia66704be7a0a8162bb85556d07b583ec756c584b
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <>
Gerrit-Reviewer: Henry Robinson <>
Gerrit-Reviewer: Michael Ho <>
Gerrit-Reviewer: Sailesh Mukil <>
Gerrit-HasComments: Yes

View raw message