zanmato1984 commented on PR #45268: URL: https://github.com/apache/arrow/pull/45268#issuecomment-2647086749
> Also, so If i'm right, can we also encapsulate failed task `PostExecuteTask` also in `TaskSchedulerImpl::ExecuteTask`? Or this should handled by outside caller This is a great question! Actually this is what I intended to do in the very beginning of my attempt to enhance this PR ([`1db3f65` (#45268)](https://github.com/apache/arrow/pull/45268/commits/1db3f653b4656c2d64b93fdff4fccb9047231fa2)). Unfortunately there is a caveat: Note the ultimate task invoking is https://github.com/apache/arrow/pull/45268/files#diff-0fd20b6ddfd91e0e5417e0c841ab3becd4ac82612647b736be3cf404aa4e2b86R371-R378, and the problem lies in the recursive call to `ScheduleMore`, which will return `Cancelled` if the scheduler has been aborted and `RETURN_NOT_OK` will shortcut all the subsequent code (including the next `ExecuteTask`). In this case, the `PostExecuteTask` inside the `ExecuteTask` will lose the chance to execute and so does the `OnTaskGroupFinished`. If you are interested, you can change the code this way and try my UT - it will fail :) -- 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]
