Henry Robinson has posted comments on this change. Change subject: IMPALA-5667: Race in DataStremSender could cause TransmitData sidecar corruption ......................................................................
Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/7436/1/be/src/runtime/data-stream-sender.cc File be/src/runtime/data-stream-sender.cc: PS1, Line 192: if (recvr_gone_) { : return Status(TErrorCode::DATASTREAM_RECVR_ALREADY_GONE); : } else if (parent_->closed_) { : return Status("DataStreamSender already closed"); Don't errors from this method propagate to the coordinator, via FragmentInstanceState::ExecInternal()->DataStreamSender::Send()? We want to avoid that, because the query should not be cancelled in the first two cases. More subtly, in the first case (recvr_gone_), other channels for this sender might still be active. We shouldn't tear down the sender until all channels are complete - except in the cancellation case where we know there's no further point doing any work. So perhaps we add a boolean method to channel called "is_complete" or "is_active", and only try to call Send on it if is_active() returns true. For the other two cases, we'd like to stop doing any more work because the whole sender is finished. I think in the past we've just relied on the fragment instance state to pick up the cancelled / closed conditions and exit the execution loop. That's probably ok to keep doing here. -- To view, visit http://gerrit.cloudera.org:8080/7436 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7742551236205c49393a7351291eae6aefb4fa09 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Sailesh Mukil <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-HasComments: Yes
