Add an example to the global_executor_concurrency docs
#50664
Conversation
guides/source/configuring.md
Outdated
| should be large enough to accommodate both the foreground threads (ie. web server or job worker threads) and background threads. | ||
|
|
||
| For each process, Rails will create one global query executor that uses this many threads to process async queries. Thus, the pool size | ||
| must be at least `(thread_count + 1) + global_executor_concurrency`. For example, if your web server has a maximum of 3 threads, |
There was a problem hiding this comment.
I know I only wrote this yesterday, but I can't remember what the + 1 is for... 🤔
There was a problem hiding this comment.
I assume that's the main/root/control thread. You really shouldn't be running queries there, but people do -- generally by having models that introspect the database at class load time.
Even still, it should be moderately difficult to have it still hold a connection by the time you're accepting work... but if you're aiming to describe the full count of threads that will exist and could possibly make use of a connection in a standard application [i.e., one that does not have any other 3rd-party threading going on], it's more technically correct to include it.
Now there's no multiplication going on, the parentheses are just confusing, though.
(IMO "must" is overly strong, at least without an accompanying "to avoid any contention" or similar -- especially if we're adding one for luck, per above.)
There was a problem hiding this comment.
Yeah, I think I must have been thinking of a control thread. But I don't remember explicitly thinking about that. Aging sucks.
- Changed "must" to "should".
- Removed the brackets.
- I don't mind keeping the
+1if it's more confusing than helpful, either way I'm pretty happy with this.
ghiculescu
force-pushed
the
global_executor_concurrency-example
branch
from
89188f0
to
b7a9463
Compare
ghiculescu
force-pushed
the
global_executor_concurrency-example
branch
from
b7a9463
to
a40bb8e
Compare
|
@byroot I think I updated based on his feedback, unless you mean to remove the “+1”? That said if this is all going to be removed soon I’m fine with closing this. I do wonder why we have a max pool size. Is it to protect the database? |
With been talking about this for so long, who know when it's gonna happen. If you were asking me if you should work on updating this doc I would have said not worth it, but now that you did the work, might as well merge it in case we still don't follow through with our plans. |
|
Okay well in that case it's ready for you (or anyone) to merge :) |
ref: #43899 (comment)