-
Notifications
You must be signed in to change notification settings - Fork 435
Implement 'quit' and 'core count' RPCs on the worker service #1833
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement 'quit' and 'core count' RPCs on the worker service #1833
Conversation
Motivation: These are 2 of the RPCs on the worker service that we need for benchmarking. Modifications: - Created the WorkerService struct that has a GRPCClient and a GRPCServer property - Implemented the 'coreCount' and 'quitWorker' RPCs Result: - The driver will be able to request 'quitWorker' and 'coreCount' from the workers
| struct WorkerService: Grpc_Testing_WorkerService.ServiceProtocol { | ||
| var testClient: GRPCClient? = nil | ||
| var testServer: GRPCServer? = nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This won't work when we add other RPCs because we need this type to be Sendable. This means we need to turn the struct into a class and protect this state using a lock. You can wrap the state up in a struct State and then use a NIOLockedValueBox to hold the state (you'll need to import NIOConcurrencyHelpers).
| if let testClient = self.testClient { | ||
| testClient.close() | ||
| } else if let testServer = self.testServer { | ||
| testServer.stopListening() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will also need to flip the atomic that we'll set on the benchmark service here.
glbrntt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
two nits, looks good otherwise
|
|
||
| @available(macOS 13.0, iOS 16.0, watchOS 9.0, tvOS 16.0, *) | ||
| final class WorkerService: Grpc_Testing_WorkerService.ServiceProtocol, Sendable { | ||
| let state: NIOLockedValueBox<State> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This, and the State struct can be private
| request: ServerRequest.Single<Grpc_Testing_WorkerService.Method.QuitWorker.Input> | ||
| ) async throws -> ServerResponse.Single<Grpc_Testing_WorkerService.Method.QuitWorker.Output> { | ||
|
|
||
| if let role = self.state.withLockedValue({ $0.role }) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think there's any reason to delay setting the $0.role back to nil, we can just do it in one go:
let role = self.state.withLockedValue {
defer { $0.role = nil }
return $0.role
}
Motivation:
These are 2 of the RPCs on the worker service that we need for benchmarking.
Modifications:
Result: