impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Henry Robinson (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4856: Port data stream service to KRPC
Date Mon, 12 Jun 2017 23:46:16 GMT
Henry Robinson has posted comments on this change.

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


Patch Set 1:

(10 comments)

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

Line 61:           bool unused = false;
> Doesn't the contract for FindRecvr() state that we need to hold 'lock_' bef
Done


Line 105:   EarlySendersList waiters;
> Add brief comment:
Done


PS1, Line 123: for (int32_t sender_id: waiters.closing_senders) recvr->RemoveSender(sender_id);
> According to the header comment in data-stream-mgr.h, a sender shouldn't be
Done


PS1, Line 300: early_senders_
> Assume the following case:
The sender fragment instance would fail, and then the coordinator should cancel the receiver.


I believe there's an outstanding issue where, if the coordinator fails to cancel a fragment
instance, the fragment instance will not fail itself. I'm going to file a JIRA for that, but
it's unrelated to KRPC.


Line 321:     // Wait for 10s
> Add a brief comment stating that this is to check if the DataStreamMgr is b
Done


http://gerrit.cloudera.org:8080/#/c/7103/1/be/src/runtime/data-stream-mgr.h
File be/src/runtime/data-stream-mgr.h:

PS1, Line 81: will do one of three things
> nit: would be nice to format them as bullet points.
Done


PS1, Line 83: if the buffer is full
> "if the batch queues are full"?
Done


PS1, Line 87: the sender
> "the sender along with its payload" ?
Done


Line 224:   /// has not yet prepared 'payload' is queued until it arrives, or is timed out.
If the
> nit: been prepared,
Done


http://gerrit.cloudera.org:8080/#/c/7103/1/be/src/service/impala-server.h
File be/src/service/impala-server.h:

PS1, Line 255: void UpdateFilter
> Leave a TODO stating that this should move to query-state.h/cc after IMPALA
I'm going to leave that for now, since I don't want to make design decisions for IMPALA-3825
in this patch.


-- 
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: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <henry@cloudera.com>
Gerrit-Reviewer: Henry Robinson <henry@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sailesh@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message