Simplify Actor Builders #1772
Merged
Simplify Actor Builders #1772
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
didier-wenzek
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
didier-wenzek
commented
Mar 2, 2023
Comment on lines
116
to
134
| impl<T, Req, Res> ServiceConsumer<Req, Res, NoConfig> for T | ||
| where | ||
| Req: Message, | ||
| Res: Message, | ||
| T: MessageSink<Res> + MessageSource<Req, NoConfig>, | ||
| { | ||
| fn get_config(&self) -> NoConfig { | ||
| NoConfig | ||
| } | ||
|
|
||
| fn set_request_sender(&mut self, request_sender: DynSender<Req>) { | ||
| self.register_peer(self.get_config(), request_sender) | ||
| } | ||
|
|
||
| fn get_response_sender(&self) -> DynSender<Res> { | ||
| self.get_sender() | ||
| } | ||
| } |
There was a problem hiding this comment.
I removed this generic implementation because too generic and preventing specializations due to Rust complaining of possible future conflicting implementations.
didier-wenzek
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
didier-wenzek
force-pushed
the
refactor/simplify-builders
branch
from
b3349ac
to
b755a9f
Compare
didier-wenzek
changed the title
didier-wenzek
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
didier-wenzek
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
didier-wenzek
force-pushed
the
refactor/simplify-builders
branch
from
b69e2ea
to
5e53735
Compare
didier-wenzek
temporarily deployed
to
Test Pull Request
GitHub Actions
Inactive
Robot Results
Passed Tests
|
albinsuresh
reviewed
Mar 6, 2023
| service: &mut impl ServiceProvider<Request, Response, Config>, | ||
| config: Config, | ||
| ) -> Self | ||
| fn connected_to(mut self, service: &mut impl ServiceProvider<Request, Response, Config>) -> Self |
There was a problem hiding this comment.
Not a big fan of this name connected_to as it doesn't really convey how it's different from connect_to without looking at the signature/doc. But, then I was wondering if we really need both connect_to and connected_to? How about just keeping this one that returns the Self? A user can choose to ignore the retuned Self when he's not interested in chaining it any further.
There was a problem hiding this comment.
Not a big fan of this name connected_to as it doesn't really convey how it's different from connect_to without looking at the signature/doc.
Would with_connection help you to grasp from the name that this creates a new enhanced builder?
Similarly, connect_to can be renamed set_connection to stress this mutates the builder.
But, then I was wondering if we really need both connect_to and connected_to? How about just keeping this one that returns the Self? A user can choose to ignore the returned Self when he's not interested in chaining it any further.
This can work by taking a &mut self and returning the same &mut self once mutated.
There was a problem hiding this comment.
If we're sticking with both the methods, then I'd prefer the with_xxx variant as it gives that "builder" feel to it.
Comment on lines
+314
to
+303
| pub struct Concurrent; | ||
| pub struct Sequential; |
There was a problem hiding this comment.
Wondering if the actor builder code would have been simpler if these structs were variants of an enum instead. I didn't really get the benefits in having them as separate concrete types.
There was a problem hiding this comment.
Enum variants are runtime things. Here, one needs to distinguish at compile time the kind of actor to build. One could also use explicit type annotations when calling the build() method - but that's not so easy as this method is used by the runtime which has no insight on which variant to build. Another option could be to use phantom types, however using these tag types seems more natural (at least to my eyes).
There was a problem hiding this comment.
Yeah, I kinda got the feeling that you wanted something static at compile time. And yes, I'd also prefer these explicit types rather than Phantom types to keep things simple.
| pub fn new(server: S, config: &ServerConfig, kind: K) -> Self { | ||
| let service_name = server.name().to_string(); | ||
| let box_builder = ServerMessageBoxBuilder::new(&service_name, config.capacity) | ||
| .with_max_concurrency(config.max_concurrency); |
There was a problem hiding this comment.
Building a concurrent message box irrespective of what the kind is?
There was a problem hiding this comment.
This is just a number at this stage. We can consider to attach this number to theConcurrent kind.
| mqtt_config: mqtt_channel::Config, | ||
| ) -> ServerActorBuilder<C8YJwtRetriever, Sequential> { | ||
| let server = C8YJwtRetriever { mqtt_config }; | ||
| ServerActorBuilder::new(server, &ServerConfig::default(), Sequential) |
There was a problem hiding this comment.
The defaultServerConfig is "concurrent". Didn't you want to override that here for a Sequential actor?
There was a problem hiding this comment.
The ServerConfig is neither concurrent nor sequential. It just provides numbers to be used by a concrete implementation.
I agree that this is misleading. One option to fix that is to add the kind (i.e. Sequential or Concurrent) to ServerConfig. I will try to do that.
| @@ -15,8 +15,8 @@ async fn get_over_https() { | |||
| } | |||
|
|
|||
| async fn spawn_http_actor() -> ClientMessageBox<HttpRequest, HttpResult> { | |||
| let mut builder = HttpActorBuilder::new().unwrap(); | |||
| let handle = ClientMessageBox::new("Tester", &mut builder.box_builder); | |||
| let mut builder = HttpActor::new().builder(); | |||
There was a problem hiding this comment.
That looks very unusual, creating a builder out of a concrete instance, as builders are typically used to build the instance. But here, it's the reverse. Something like HttpActor::builder() would have been more natural.
There was a problem hiding this comment.
Agree - even if I don't read the same way as you (getting a builder from an instance). Will see why I did that and how this can be changed.
So a ServiceConsumer can be connected to a ServiceProvider without any knowledge of the config used by the former to connect the latter. Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
This function was too specialized and only usefull for testing. Now tests will have to build message boxes exactly the same way as done using actors. This adds a bit of burden on testing but reduce the number of concepts as well as the gap between test and production code. Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
The actor builders of server actors (e.g HttpActor, JwtRetriever, ScriptActor) share the same pattern. This pattern is now implemented as a type: ServerActorBuilder that handles both sequential and concurrent servers. Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
didier-wenzek
force-pushed
the
refactor/simplify-builders
branch
from
5e53735
to
5e93286
Compare
didier-wenzek
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
In order to stick with builder usual terms, the following methods have been renamed: - `ServiceConsumer::connected_to` as `ServiceConsumer::with_connection` - `ServiceConsumer::connect_to` as `ServiceConsumer::set_connection` - `ServiceProvider::connect_with` as `ServiceProvider::add_peer` Futhermore, the type signature of `ServiceConsumer::set_connection` has been changed to return a `&mut Self` allowing methods calls to be chained. For now, both methods `set_connection` and `with_connection` are kept. One might consider to deprecate one - as their usage are close. Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
didier-wenzek
temporarily deployed
to
Test Pull Request
GitHub Actions
Inactive
albinsuresh
approved these changes
Mar 7, 2023
44 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Proposed changes
WIP
So a ServiceConsumer can be connected to a ServiceProvider
without any knowledge of the config used by the former to connect the latter.
with_c8y_http_proxy) with generic invocations ofconnect_to.Types of changes
Paste Link to the issue
#1724
Checklist
cargo fmtas mentioned in CODING_GUIDELINEScargo clippyas mentioned in CODING_GUIDELINES