GH-15243: [C++] fix for potential deadlock in the group-by node #33700
Conversation
|
|
|
This is a spot fix. I suspect there are other potential issues in other nodes but we just don't have tests for them. #15253 is a more thorough fix but I don't believe it will be finished in time for the release. |
|
@github-actions crossbow submit test-ubuntu-18.04-cpp-static |
|
Revision: 20629be Submitted crossbow builds: ursacomputing/crossbow @ actions-681afd8c3f
|
| if (ErrorIfNotOk(Consume(ExecSpan(batch)))) return; | ||
|
|
||
| if (input_counter_.Increment()) { | ||
| ErrorIfNotOk(OutputResult()); |
There was a problem hiding this comment.
So we don't want to propagate the error back anymore? (I suppose that's all going away anyways?)
There was a problem hiding this comment.
Yes, at the moment, the only thing that handles this error is the sink node and all it will do is trigger an abort which will end up coming back here and marking this finished anyways.
That used to be needed because it was the only way to trigger an abort on a failure.
Now, with the async scheduler, any failed task will trigger an abort. Furthemore, since finished_ is a "task" it will trigger the abort in this path.
| num_output_batches)); | ||
| Status st = | ||
| plan_->query_context()->StartTaskGroup(output_task_group_id_, num_output_batches); | ||
| if (st.IsCancelled()) { |
There was a problem hiding this comment.
nit: this if-else seems redundant, since OutputResult will MarkFinished even if we don't do this?
There was a problem hiding this comment.
This is translating a failed status into a successful status (the test expects a plan to succeed after calling StopProducing).
There was a problem hiding this comment.
The else condition propagates the failed status.
* Closes: #15243 Authored-by: Weston Pace <[email protected]> Signed-off-by: Weston Pace <[email protected]>
2 participants
ExecPlanExecution.StressSourceGroupedSumStoptimeout #15243