Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201701031925.v03JPeq3006492@ip-10-146-233-104.ec2.internal>
Date: Tue, 3 Jan 2017 19:25:40 +0000
From: "Henry Robinson (Code Review)" <gerrit@cloudera.org>
To: Sailesh Mukil <sailesh@cloudera.com>, impala-cr@cloudera.com,
        reviews@impala.incubator.apache.org
CC: Lars Volker <lv@cloudera.com>
Reply-To: henry@cloudera.com
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_IMPALA-3977=3A_TransmitData=28=29_should_not_block=0A?=
In-Reply-To: <gerrit.1481656464000.I47f9d6c1de95a72ab73471dcd2a3118ef13e7db0@gerrit.cloudera.org>
References: <gerrit.1481656464000.I47f9d6c1de95a72ab73471dcd2a3118ef13e7db0@gerrit.cloudera.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Content-Disposition: inline
User-Agent: Gerrit/2.12.2
archived-at: Tue, 03 Jan 2017 19:25:51 -0000

Henry Robinson has posted comments on this change.

Change subject: IMPALA-3977: TransmitData() should not block
......................................................................


Patch Set 4:

(13 comments)

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

PS4, Line 42: 
Unfortunately you can't just remove flags, because then Impala won't start if someone set them. Instead, change the comment to "DEPRECATED", and leave a TODO to remove in the next compat-breaking release.


PS4, Line 117:  == true
remove


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

PS4, Line 53: This does not block
Mention that senders are expected to try again in this instance.


PS4, Line 151: bool* is_queue_full
Just return a boolean?


Line 167:     *is_queue_full = true;
I thought you were going to add a timed-wait here.


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

Line 220:   system_time timeout = get_system_time() +
Why not use MontonicMillis() for this?


PS4, Line 223: at-least
"at least".

But in general this comment is a bit confusing. We don't want to give the impression that the RPC could logically be sent (or really, received) more than once. Instead, you should spell out the state machine that TransmitData() now follows in a single comment, possibly at the definition of TransmitData(), or maybe in this method. It's complicated enough that we should document it in one place, rather than across several comments.


PS4, Line 245: DATASTREAM_RECEIVER_EAGAIN
Now that it's clearer when this could be received I'd rename it to DATASTREAM_RECEIVER_NOT_READY


PS4, Line 249: else
Don't need else since previous block returns.


PS4, Line 252: shutdown
shut down


Line 265:   SleepForMs(DATASTREAM_RPC_RETRY_SLEEP_DURATION_MS);
indentation


Line 270:   rpc_status_ = Status(TErrorCode::DATASTREAM_SENDER_TIMEOUT, PrintId(fragment_instance_id_));
long line


http://gerrit.cloudera.org:8080/#/c/5491/4/be/src/runtime/data-stream-test.cc
File be/src/runtime/data-stream-test.cc:

PS4, Line 574: // Set a higher timeout for this test.
             :   FLAGS_datastream_sender_timeout_ms = 5000;
This will affect all tests. Consider using ScopedFlagSetter (see webserver-test.cc).


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I47f9d6c1de95a72ab73471dcd2a3118ef13e7db0
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sailesh@cloudera.com>
Gerrit-Reviewer: Henry Robinson <henry@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sailesh@cloudera.com>
Gerrit-HasComments: Yes