[GitHub] [arrow] westonpace commented on pull request #10233: ARROW-12641: [C++] Provide thread local state for tasks in a thread pool

[GitBox <gi...@apache.org>]

westonpace commented on pull request #10233:
URL: https://github.com/apache/arrow/pull/10233#issuecomment-833010580


   Ok, I think that makes sense then.
   
   * I wouldn't name it `MakeThreadLocalState`.  "Thread local" has a pretty well understood existing definition that isn't really this.  Also, the lifetime of the `BorrowSet` is independent of the lifetime of the thread (it is more tied to the lifetime of a "workflow").  Maybe call it "workflow" state?
   * Outside of a some callbacks I don't think this logic belongs in `thread_pool.h`/`executor.h`.  You could put the `MakeThreadLocalState` function in `borrowed.h` instead (and simply take `Executor*` as an argument).  People who are working on schedulers and thread pools probably don't want to have to know about borrow sets.
   * I can't speak to the merits of any performance benefit / complexity tradeoff but I can see where it could theoretically be advantageous to use a borrow set.


-- 
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:
users@infra.apache.org