L30-Ability-to-restrict-max-threads-in-C++SyncServer.md#84
Conversation
| ``` | ||
| * Max threads are set to INT_MAX by default | ||
| * In the initial implementation, I plan to divide the max_threads equally among multiple `ThreadManager` objects (there are as many `ThreadManager` objects as the number of server completion queues). It is not clear on which strategy is better at this point | ||
| - (1) Have all thread managers create threads from a common pool (but potentially starving some thread managers and also making the quota-check a potential global contention point) |
There was a problem hiding this comment.
Is thread creation a common enough occurrence that there's worry about contention?
There was a problem hiding this comment.
It could be if there are multiple server completion queues (and even though right now a Sync server creates just one server completion queue, in future we probably want to increase it to be equal to the number of cores).
FWIW, since we only have one server completion queue currently, both the strategies above will effectively be the same..
| /// new_max_threads. There is no time bound on when this may happen i.e none | ||
| /// of the current threads are forcefully destroyed and all threads run their | ||
| /// normal course. | ||
| ResourceQuota& SetMaxThreads(int new_max_threads); |
There was a problem hiding this comment.
Is 0 a valid value for max threads? Is that how I say "I will never use this server synchronously. Do not create any threads. I'll be polling completion queues myself"?
There was a problem hiding this comment.
0 is a valid value but that will effectively make the server not respond to any incoming requests. So it is not a good idea to initialize max_threads to 0. However, if for whatever reason, the application suddenly decides to stop accepting incoming requests for sometime, it can set the max_threads to 0.
There was a problem hiding this comment.
btw, as I am thinking more about this, it might be easier to just implement option (1) at this point. This way, when applications resize the max_threads by calling ResourceQuota::SetMaxThreads, it is easier to make those changes visible to all thread managers..
There was a problem hiding this comment.
..and oh to answer your other question
" Is that how I say "I will never use this server synchronously. Do not create any threads. I'll be polling completion queues myself"?"
No, actually the server completion queues used by the sync server are not exposed to the application. So there is no question of the application trying to poll the queues itself.
If application wants to poll itself, it has to create its own completion queues an poll them ..
There was a problem hiding this comment.
Can you start the grpc discussion thread for this proposal? We should move extended comments there.
There was a problem hiding this comment.
| * Status: In Review | ||
| * Implemented in: C++ | ||
| * Last updated: June 6, 2018 | ||
| * Discussion at: |
There was a problem hiding this comment.
Can you please start a discussion thread on grpc-io about this and add the URL? Most of the technical questions should actually belong there.
| Each of the `ThreadManager` objects in a given server check with the Server's resource quota object when creating new threads and will not create new threads if no quota is available. | ||
|
|
||
| ## Details | ||
| More concretely, the following API changes are being proposed |
There was a problem hiding this comment.
Can you take the code details out of the document and put them in a PR linked from the document instead? I like to add that in the "Implemented In" line in the header
There was a problem hiding this comment.
o you mean the private API details ? - the other code snippets are public API changes and I thought should be in this document..
There is no PR for this yet (I mean, I have an in-progress change in my local branch but I did not implement the bulk of the changes yet. I want to get some agreement on the API before proceeding.
…-Ability-to-restrict-max-threads-in-C++SyncServer.md
sreecha
changed the title
|
I'm approving based on the current content, but the discussion thread will remain open. The earliest possible merge date for this proposal is June 20. |
Minor rewording and fixed a typo.
|
Sending another approval to confirm that I still approve. |
3 participants
No description provided.