feat: configurable strategy for background threads #955
Conversation
googlebot
added
the
cla: yes
Codecov Report
@@ Coverage Diff @@
## master #955 +/- ##
==========================================
+ Coverage 94.27% 94.56% +0.28%
==========================================
Files 92 94 +2
Lines 3932 3953 +21
==========================================
+ Hits 3707 3738 +31
+ Misses 225 215 -10
Continue to review full report at Codecov.
|
This change introduces an interface to represent the background thread(s) used by a connection. I included two implementations in this PR, one that automatically creates a single background thread, and another that uses one or more threads provided by the application. The ConnectionOptions class provides a factory for `BackgroundThreads`, for those (we expect few) applications that will want to control the background threads.
| * Connections need to perform background work on behalf of the application. | ||
| * Normally they just create a background thread and a `CompletionQueue` for | ||
| * this work, but the application may need more fine-grained control of their | ||
| * threads. In this case the application can provide the |
| virtual grpc_utils::CompletionQueue cq() const = 0; | ||
|
|
||
| /// Terminate any automatically created threads. | ||
| virtual void Shutdown() = 0; |
There was a problem hiding this comment.
Does Shutdown() need to exist as a part of this interface, or can a subclass just do the shutdown work in their destructor if there is any?
There was a problem hiding this comment.
I am not sure. I think we will want to shut it down explicitly. But let's be conservative, I have removed the member function, we can always add it later if we really need it.
|
|
||
| using BackgroundThreadsFactory = | ||
| std::function<std::unique_ptr<BackgroundThreads>()>; | ||
| BackgroundThreadsFactory background_threads_factory() { |
There was a problem hiding this comment.
I'm not sure I understand how to use this method. It looks like every call to this getter will return a factory that, when invoked, will create a new "BackgroundThread", and by default, will use a new CompletionQueue. Is that right?
Would it be good to just expose access to a single CompletionQueue instance that everyone can use? But honestly, maybe I'm just not understanding how this works.
There was a problem hiding this comment.
I'm not sure I understand how to use this method. It looks like every call to this getter will return a factory that, when invoked, will create a new "BackgroundThread", and by default, will use a new CompletionQueue. Is that right?
Yes.
Would it be good to just expose access to a single CompletionQueue instance that everyone can use? But honestly, maybe I'm just not understanding how this works.
I think we decided to tie the lifetime of the automatically created threads to the lifetime of the spanner::*Connection object. That means the *Connection needs to know if it creates a thread and completion queue, and if it does create them it needs to shutdown the completion queue and join the thread in its destructor. All those ifs suggested that this was a strategy given to the *Connection object.
In other words, we need to return something more than just the CompletionQueue, we need either (a) a thing that creates (when needed) the background thread for you, or (b) a bool that tells you if you should create a thread.
I went with option (a) because it seems wasteful to have every spanner::*Connection implement the "If I was asked to create a thread do this, otherwise do that".
|
PTAL |
…-cloud-cpp-spanner#955) This change introduces an interface to represent the background thread(s) used by a connection. I included two implementations in this PR, one that automatically creates a single background thread, and another that uses one or more threads provided by the application. The ConnectionOptions class provides a factory for `BackgroundThreads`, for those (we expect few) applications that will want to control the background threads.
This change introduces an interface to represent the background
thread(s) used by a connection. I included two implementations in this
PR, one that automatically creates a single background thread, and
another that uses one or more threads provided by the application.
The ConnectionOptions class provides a factory for
BackgroundThreads,for those (we expect few) applications that will want to control the
background threads.
This fixes #606
This change is