-
Notifications
You must be signed in to change notification settings - Fork 336
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
[WIP] 255 standard dependency injection #260
Conversation
@chleit Thanks for the contribution and first draft, I will review these. |
/// <summary> | ||
/// Gets the identity of this actor. | ||
/// </summary> | ||
/// <value>The <see cref="ActorId"/> for the actor.</value> | ||
public ActorId Id { get; } | ||
public ActorId Id { get; private set; } |
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.
How would user provide its own constructor for Actor? In its constructor, ActorId will not be available>
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 think there are several ways for that:
- Make the setter for ActorId public - I wouldn't do that
- ActorId is currently set in OnActivateInternalAsync() - make that method public
- Extract the code that was originally in the Actor() constructor from OnActivateInternalAsync() to a public method ConfigureActor(actorId, actorService, ...)
I personally would go for option 3.
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 know this is kind of "not object oriented programming" because the Actor would not be fully instantiated by the constructor but would need an extra ConfigureActor() call.
But the problem with this basic kind of IoC container IServiceProvider implements, that AFAIK you can't easily inject instance runtime information (like an ActorId).
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.
User does not know the actor id, it needs to be provided in the constructor when a call comes for it, OnActivateAsync seems too late in the actor lifecyle.
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.
OK, I have to differentiate:
- In normal (DAPR) operation, the Actor is instantiated and activated in the ActorManager:
internal async Task ActivateActor(ActorId actorId)
{
// An actor is activated by "Dapr" runtime when a call is to be made for an actor.
var actor = this.actorService.CreateActor();
await actor.OnActivateInternalAsync(this.actorService, actorId);
...
}
So there's practically no difference between providing the ActorService and ActorId in the constructor or in the call to OnActivateInternalAsync(). So we also could easily have the instantiation and activation like this:
internal async Task ActivateActor(ActorId actorId)
{
// An actor is activated by "Dapr" runtime when a call is to be made for an actor.
var actor = this.actorService.CreateActor();
actor.Configure(this.actorService, actorId);
await actor.OnActivateInternalAsync();
...
}
- For unit-testing the user has to provide the ActorService (or a mock thereof) and an ActorId. So for that we need same public methode like actor.Configure(...)
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.
Let me think a bit more about it, when the user actor constructor is called, they can choose to load things based on the actor id.
IMO, public Configure method for mocking doesnt seem like the right thing to do.
@chleit just fyi, sorry for the delay, I am still looking at it. |
cc @malotho as well. |
Description
Implemented changes to support standard .Net Core Dependency Injection
@amanbha: First draft for the changes we discussed yesterday. Tests are partially commented out. Manual test was #successful.
Issue reference
#255
Checklist
Please make sure you've completed the relevant tasks for this PR, out of the following list: