impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dan Hecht (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-4856: Port data stream service to KRPC
Date Mon, 30 Oct 2017 19:35:45 GMT
Dan Hecht has posted comments on this change. ( )

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

Patch Set 6:


Some initial comments for this round.
File be/src/exec/kudu-util.h:
PS6, Line 45: status_
nit: use same mangling between this and KUDU_RETURN_IF_ERROR (prepend seems better too since
it's not a member of a class and status_ is a comment member name).
File be/src/runtime/krpc-data-stream-mgr.h:
PS6, Line 97: two things:
            : /// process it immediately, add it to a fixed-size 'batch queue' for later processing,
            : /// or defer processing the RPC if the 'batch-queue' is full
i'm confused what the "two" cases are from this comment.  Also, I think it's kind of confusing
what "processing" means. Should it read something like:

two things: deserialize it immediately adding it to the a 'batch queue', or defer deserializing
and respond to the RPC later if the 'batch queue' is full.

Also, the batch queue isn't really "fixed size", right?
File be/src/runtime/
PS6, Line 171:     if (!blocked_senders_.empty()) {
we talked about this in person, but want to note it so I don't forget: this loop will deque
a "random" number of blocked senders, until the first one happens to finish deserializing
and shows up in batch_queue_. That seems wrong.
PS6, Line 220: DCHECK_GT(num_remaining_senders_, 0);
why do we have this DCHECK (i.e. why is this condition important here), and could it be violated
with out of order RPCs?
PS6, Line 237: recvr_->ExceedsLimit(batch_size)
it seems like we should be ORing that with !block_senders_.empty(), or something. Otherwise,
stuff that's been in the block_senders_ queue for a while can be passed by new stuff. i.e.
block_senders_ senders can get starved -- maybe that explains longer than expected RPC times
we've seen in some cases?

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: 6
Gerrit-Owner: Michael Ho <>
Gerrit-Reviewer: Dan Hecht <>
Gerrit-Reviewer: Michael Ho <>
Gerrit-Reviewer: Mostafa Mokhtar <>
Gerrit-Reviewer: Sailesh Mukil <>
Gerrit-Comment-Date: Mon, 30 Oct 2017 19:35:45 +0000
Gerrit-HasComments: Yes

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