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 <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-HasComments: Yes
