Pranav Lodha has posted comments on this change. ( http://gerrit.cloudera.org:8080/22506 )
Change subject: Stop the fragment if all exchange receivers are closed ...................................................................... Patch Set 2: (3 comments) > Patch Set 2: > > Build Successful > > https://jenkins.impala.io/job/gerrit-code-review-checks/19990/ : Initial code > review checks passed. Use gerrit-verify-dryrun-external or > gerrit-verify-dryrun to run full precommit tests. Thanks Joe for the patch! http://gerrit.cloudera.org:8080/#/c/22506/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/22506/2//COMMIT_MSG@7 PS2, Line 7: Stop the fragment if all exchange receivers are closed I think this line is redundant with the commit title. http://gerrit.cloudera.org:8080/#/c/22506/2//COMMIT_MSG@16 PS2, Line 16: . Maybe add a Testing header when all the core tests pass? http://gerrit.cloudera.org:8080/#/c/22506/2/be/src/runtime/krpc-data-stream-sender.cc File be/src/runtime/krpc-data-stream-sender.cc: http://gerrit.cloudera.org:8080/#/c/22506/2/be/src/runtime/krpc-data-stream-sender.cc@540 PS2, Line 540: if (!remote_recvr_closed_) { : remote_recvr_closed_ = true; : parent_->num_closed_channels_++; : if (parent_->num_closed_channels_.load() == parent_->GetNumChannels()) { : for (int i = 0; i < parent_->GetNumChannels(); ++i) { : DCHECK(parent_->channels_[i]->IsRecvrClosed()); : } : parent_->eos_ = true; : } : } Should we consider adding a log message when all channels are closed? -- To view, visit http://gerrit.cloudera.org:8080/22506 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I02a3c507755d0c15994dc0fbfdb4f6540ddf16ba Gerrit-Change-Number: 22506 Gerrit-PatchSet: 2 Gerrit-Owner: Joe McDonnell <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Pranav Lodha <[email protected]> Gerrit-Comment-Date: Thu, 21 Aug 2025 21:44:52 +0000 Gerrit-HasComments: Yes
