impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michael Ho (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-4856: Port data stream service to KRPC
Date Tue, 07 Nov 2017 21:49:49 GMT
Michael Ho has posted comments on this change. ( )

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

Patch Set 9:

File be/src/runtime/krpc-data-stream-mgr.h:
PS8, Line 435: 'num_requ
> 'num_request' requests
File be/src/runtime/
PS8, Line 62: ds, C
> how was that chosen? do we have a test case that causes this queue to fill 
Just a large enough number to reduce the chance of the queue filling up.  Yes, need to come
up with a test case to fill up this queue.
PS8, Line 109:       // Let the receiver take over the RPC payloads of early senders and process
             :       // asynchronously.
             :       for (unique_ptr<TransmitD
> this comment seems out of place. this is more an implementation detail of t
File be/src/runtime/krpc-data-stream-recvr.h:
PS8, Line 129: n
> and start a deserialization task to process it asynchronously.
PS8, Line 130: ask to p
> this "transfer" is in the oppose direction of how our "Transfer" methods us
Renamed to TakeOverEarlySender().
PS8, Line 197: eive
> cpu time or wall time?
wall-clock time.
PS8, Line 200: _;
> same question
wall-clock time.
File be/src/runtime/
PS8, Line 67: data
> the resources
PS8, Line 72: imit, the
> RPC state
PS8, Line 73: nto 'deferred_
> we shouldn't normally refer to private fields in public class comments, but
PS8, Line 87:   void TakeOverEarlySender(std::unique_ptr<TransmitDataCtx> ctx);
> same comment as recvr header comment.
PS8, Line 195:   // cur_batch_ must be replaced with the
> I don't think we need this loop. see other comments in this function.
PS8, Line 197: atch = nullptr;
> nit: consider swapping the order of these so that the fast case comes first
PS8, Line 210:     }
> nit: i think we could do without some of the blank lines in this method to 
I leave a blank line between each loop exiting conditions.
PS8, Line 211: 
> Actually, I think the loop exiting condition is not quite right, which led 
There is an invariant that EOS cannot be sent by a sender when there is outstanding TransmitData()
RPC so we should be able to get by by just checking for the termination condition of: (num_remaining_senders_
== 0 && batch_queue_.empty())
PS8, Line 218: 
> given that we just checked the other two loop exit conditions, isn't this d
Converted to DCHECK().
PS8, Line 229: d
> to parallelize the CPU bound deserialization work.
PS8, Line 239:     }
> once you get rid of the loop, I think you'll be able to eliminate this unlo
PS8, Line 389:       status.ToProto(ctx->response->mutable_status());
> at this point, 'ctx' effectively takes ownership, right? we should add a co
PS8, Line 406:     const RowBatchHeaderPB& header = ctx->request->row_batch_header();
             :     AddBatchWork(batch_size, header, tuple_offsets
> nit: let's reorder these two lines since num_deferred_batches_ is counting 
File be/src/runtime/
PS8, Line 434:   resp_.Clear();
> May want to call resp_.Clear() too.

To view, visit
To unsubscribe, visit

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic0b8c1e50678da66ab1547d16530f88b323ed8c1
Gerrit-Change-Number: 8023
Gerrit-PatchSet: 9
Gerrit-Owner: Michael Ho <>
Gerrit-Reviewer: Dan Hecht <>
Gerrit-Reviewer: Michael Ho <>
Gerrit-Reviewer: Mostafa Mokhtar <>
Gerrit-Reviewer: Sailesh Mukil <>
Gerrit-Comment-Date: Tue, 07 Nov 2017 21:49:49 +0000
Gerrit-HasComments: Yes

  • Unnamed multipart/alternative (inline, 8-Bit, 0 bytes)
View raw message