-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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 POCO Grains #7351
Comments
My first reaction was that this change would be great, mainly because it will make it easier for people like us using Domain Driven Design to turn their core domain model entities into grains as simply as by implementing Grain interfaces. The requirement to inherit from the Grain base class makes it impossible right now, so there is a need to introduce an additional Grain layer in the project structure, wrapping and seemingly duplicating the core domain model. However, thinking about it deeply, I am not sure this would simplify the life of developers that much in practice because let's remember that Grain methods must be async - and therefore return Tasks. Therefore if one wants to use his core domain model as grains, he will need to extend the interface of the domain entities with additional async methods essentially wrapping the core methods (returning non-async values). So there'd be the same code duplication, but the class would now look more messy and arguably have too much responsibility. Now If the programmer decides to treat grains as first-class citizens by making all his domain model entities expose async interfaces by default, then inheriting from a Grain base class is not a big deal since his design his already influenced by Orleans anyway. My point is that the quality-of-life gains may be negligible in practice. And the quality of life may even be degraded if the Grain base class is removed and the developer now needs to manually do plumbing that used to be done automatically for him in the base class. With this regard, removing the magic perception should not be done at the cost of degrading quality of life (and for the records, I am among those seeing Orleans as magic 😄 ). I therefore tend to think that it may actually be a good thing to have this base class, to facilitate quality of life conveniences, and to encourage developers to follow the single responsibility principle. With this regard, to me the fact that Grains must derive from the Grain base class actually is a reminder that Grains "are implementations of some user-defined communication interfaces". As a magic practioner lifting that requirement would tend to make me think that Orleans is some solid white magic stuff being able to send any arbitrary unserializable class over the wire. |
Thanks for the thoughtful response, @nkosi23. I also believe that he Grain base class should remain: it's very convenient. For serialization, Orleans currently now requires My general take is that helpers (like EDIT: I accidentally closed the issue - typing on mobile |
I see 2 more advantages: Less couplingI think a lot of behavior is too coupled to the core at the moment and it would be easier to keep it outside.
This does not only improve the architecture but could also motivate developers to build more extension and pluggable behavior for grains. I have 2 examples:
TestabilityI thin grains are too hard to test at the moment. You have to provide several mocks with unclear purpose and some methods like DeactivateOnDelay just throw exceptions in tests. It would be easier to just inject a few services and mock them when needed. |
TLDR1; Let us have: public interface IGrain
{
IGrainContext Context { get; }
} And we can do: public class MyOwnGrainBase: IGrain
(
public MyOwnGrainBase(IGrainContext context)
{
Context = context;
}
) TLDR2; Or even better, just let us have: public class MyOwnGrainBase
{
[GrainContext]
public IGrainContext Context { get; set; }
} Time for some devil's advocacy. I think the concept of "POCO grains" itself can lead to very chatty designs, when developers just take Orleans for a caching framework and nothing else. I've seen this manifest with Akka as well, and you end up with designs that are no different from a vanilla aspnet application with code and data still separate and moving through the network all the time. While these designs work, they miss the point and benefit of using actor frameworks, as we can achieve the same effect with Redis with less code. In my opinion, all this is an effect of marketing rather than technology, and I tend to push back against using actor frameworks just for POCO purposes, stressing that the benefit comes from having data and behavior together in the same place, one analogy being the benefits of stored procedures in databases to minimize network chatter and latency. Devil's advocacy done now, I believe, in general, design changes that align it closer to aspnet web api standards can make Orleans easier to learn for dotnet developers, as I suspect many of them will already have been exposed by aspnet by the time they learn about Orleans. The aspnet web api framework is reliant on attributes and other declarative programming artifacts and this appears to be well-accepted, as far as I can tell, provided there's no hidden conventions to confuse and surprise people. Like Following this train of thought, Blazor is another example where the team has embraced property based injection, now not only for the framework itself but for the developer as well, using the With all the above in mind, I can see an approach where developers can just mark some property with a |
@JorgeCandeias wrote
I considered this, but it's a little troublesome, which is why I proposed the optional Using attributes, such as I prefer we avoid magic property injection. One of the purposes of POCO grains is to have something which does not rely on magic. The POCO grains aren't a convenience feature. |
If zero magic is the objective then how about accepting a context object in cases such as public class MyGrain: ISomeUserInterface, ISomeInternalUserInterface
{
private readonly IGrainContext _context;
public MyGrain(IGrainContext context)
{
_context = context;
}
public Task SomeWorkAsync()
{
var myself = this.AsReference<ISomeInternalUserInterface>(_context);
return Task.CompleteTask;
}
} This gives us redundant code and no convenience, but also gives us zero magic, and unit test friendlyness. The above said, I don't know how would Orleans figure out that this class is supposed to be a grain, without the use of, at the very least, some magic I don't see any issue with having an accessible |
I think that's a viable alternative, @JorgeCandeias. or alternatively, developers can use |
I think
is strange for tests. How do you mock it? Not very obvious in contrast to something like
|
How about the following, for the issue of mapping from a grain instance back to its
|
I've opened a PR for a work-in-progress implementation of "POCO" grains at #7360 - please let us know what you think. |
Currently, all grain implementations must derive from
Grain
.This issue is about lifting that requirement.
Motivation
Why remove
Grain
as a required base class? The primary reason, as I see it, is not practical at all. It's simply about removing what some may perceive as magic. When developers suspect magic is involved, it tends to cloud/inhibit reasoning abilities. Grains are not at all magic: they are simply implementations of some user-defined communication interfaces. TheGrain
base class is very thin: it largely just provides helpers to access other facilities (timers, streams, grain factory, identities, etc).There are some practical reasons. For example, .NET classes can only have a single base class, which means that business logic which is Orleans-independent cannot be implemented as a grain by simply sub-classing and adding (eg) an interface which signifies that a class is a grain.
Progress so far
In #6585, we re-oriented the runtime around
IGrainContext
, which is a type the runtime uses to represent a grain activation. It's theIGrainContext
which holds a reference to yourGrain
classes and which is responsible for all interactions with your grain implementation (activation, deactivation, message scheduling).In #6446, @KevinCathcart implemented a proof of concept for how POCO grains could work - that lays a lot of groundwork and provides philosophy/guidance for how this should work as well as the potential issues which may require API changes.
There is a work-in-progress implementation: ReubenBond@2d09b70 (currently found in this branch, which also demonstrates "RPC Services" - multi-threaded/reentrant stateless services: https://github.com/ReubenBond/orleans/commits/feature/rpc-services)
Remaining work / discussion
Rather little, it's mostly about decision making and discussion. What remains is to remove some checks and casts to
Grain
and re-orient some remaining types (eg,IGrainRuntime
, which exists primarily for unit testing) aroundIGrainContext
instead ofGrain
.AsReference<T>()
We also need some way to continue being able to support calling
this.AsReference()
andthis.AsReference<T>()
from a grain - extension methods which lets a developer get a reference to the grain which called the method. The methods currently rely on the caller deriving fromGrain
so that it can access the correspondingIGrainContext
(Grain
has an internalData
property which points to the context). IfGrain
is no longer required, we need some other way to enable this method. One solution is to allow developers to implement a new interfaceIGrainBase
which has a single propertyIGrainContext Context { get; }
. See this comment for more context: #6446 (comment)API / ergonomics concerns
Should we require that developers signify that a type is a grain implementation somehow, or should we use logic that "any non-abstract class which implements the
IGrain
marker interface (no methods) is a grain implementation"?Alternatives (among others) could be:
IGrainBase
marker interface - noting thatIGrain
is already used today for grain interfacesThe WIP branch linked above just uses the
IGrain
requirement. It only supports.AsReference<T>()
if the grain happens to implementIGrainBase
.Example API
With
IGrainBase
to support.AsReference()
(via theGrainContext
property),OnActivateAsync
,OnDeactivateAsync
:Feedback and discussion appreciated
The text was updated successfully, but these errors were encountered: