westonpace commented on a change in pull request #9644:
URL: https://github.com/apache/arrow/pull/9644#discussion_r601733537
##########
File path: cpp/src/arrow/util/thread_pool.h
##########
@@ -104,15 +104,22 @@ class ARROW_EXPORT Executor {
template <typename T>
Future<T> Transfer(Future<T> future) {
auto transferred = Future<T>::Make();
- future.AddCallback([this, transferred](const Result<T>& result) mutable {
+ auto callback = [this, transferred](const Result<T>& result) mutable {
auto spawn_status = Spawn([transferred, result]() mutable {
transferred.MarkFinished(std::move(result));
});
if (!spawn_status.ok()) {
transferred.MarkFinished(spawn_status);
}
- });
- return transferred;
+ };
+ auto callback_factory = [&callback]() { return callback; };
+ if (future.TryAddCallback(callback_factory)) {
+ return transferred;
+ }
+ // If the future is already finished and we aren't going to force spawn a
thread
+ // then we don't need to add another layer of callback and can return the
original
+ // future
Review comment:
I agree with the new API. C# has that API as well (Task::ContinueWith
can take a scheduler) so there is some precedence. I'll add a follow-up PR. I
don't believe it is necessary for the current work though.
You are correct, the old method could possible introduce a new thread task
spawned in some situations where this doesn't (e.g. if the callback is added
very quickly after calling transfer before the TP has a chance to schedule the
"mark finished" task). I don't really see any reason this is desirable as it
can only mean more thread pool tasks without any benefit. So I will leave this
as is for now.
--
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
