westonpace commented on code in PR #15104:
URL: https://github.com/apache/arrow/pull/15104#discussion_r1059425521
##########
cpp/src/arrow/compute/exec/sink_node.cc:
##########
@@ -381,19 +388,11 @@ class ConsumingSinkNode : public ExecNode, public
BackpressureControl {
protected:
void Finish(const Status& finish_st) {
- plan_->query_context()->async_scheduler()->AddSimpleTask([this,
&finish_st] {
- return consumer_->Finish().Then(
- [this, finish_st]() {
- finished_.MarkFinished(finish_st);
- return finish_st;
- },
- [this, finish_st](const Status& st) {
- // Prefer the plan error over the consumer error
- Status final_status = finish_st & st;
- finished_.MarkFinished(final_status);
- return final_status;
- });
- });
+ if (finish_st.ok()) {
+ plan_->query_context()->async_scheduler()->AddSimpleTask(
+ [this] { return consumer_->Finish(); });
Review Comment:
We now mark the node finished_ inside the Finish method, when we are
scheduling the finish task. This is perhaps slightly inaccurate for telemetry
purposes but close enough and avoids the complexity of the old implementation.
The whole concept of `finished_` is pretty extraneous at this point ([and
going away
soon!](https://github.com/apache/arrow/commit/4200023ec3f164958c746c18ad75bebb91906315)).
A plan is finished when it runs out of tasks. The concept of "node finished"
at this point, is just something used to cause deadlocks :). We can actually
mark the sink node finished in `StartProducing` and everything will still work
but it may still serve some marginal value for telemetry until I clean it up
(and I want to clean up telemetry soon too anyways).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
