-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Non-static GrainClient #2822
Non-static GrainClient #2822
Conversation
6d44c64
to
ed46fbe
Compare
I've tagged this as a work-in-progress for now. The biggest change intended for the final version is to make the client startup asynchronous |
c965ccd
to
79d3bc5
Compare
53fc1fc
to
b4ec494
Compare
Alright, the |
What is the reasoning for having to call Start? Should we separate the creation of the ClusterClient into something that returns an already initialized client? Close(Async) still makes sense, kind of like an async Dispose method, but it sounds like initializing shouldn't be done on the ClusterClient. What do you think? |
For example, consider this flow:
That flow has no chance of race conditions, whereas the following does
Additionally, I believe creation should be synchronous, whereas connecting to a cluster is async. I'd be happy if we had a ClientBuilder, but I figured that change should coincide with a similar change on the silo side and at the same time, we should allow the user to provide their own DI container (and inject their own services). |
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.
Some small comments I started to write. Sending them since we also synced in person. I'll continue reviewing anyway.
src/Orleans/Core/ClusterClient.cs
Outdated
remove | ||
{ | ||
this.ThrowIfDisposed(); | ||
this.runtimeClient.ClusterConnectionLost -= value; |
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 shouldn't throw, right? Should be safe to remove the event handler in any order
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'm not sure what the norm is, but as of the latest commit, this event is no longer public (I kept it on IInternalClusterClient
). I've removed the throw.
src/Orleans/Core/ClusterClient.cs
Outdated
/// Initializes a new instance of the <see cref="ClusterClient"/> class. | ||
/// </summary> | ||
/// <param name="configuration">The client configuration.</param> | ||
public ClusterClient(ClientConfiguration configuration) |
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.
private to favor the factory method (or builder in the future)?
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.
done - I made the class internal
src/Orleans/Core/ClusterClient.cs
Outdated
this.ThrowIfDisposed(); | ||
using (await this.initLock.LockAsync()) | ||
{ | ||
await this.runtimeClient.Start(); |
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.
no checks to see if it's already started? or is runtime client idempotent?
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.
BTW, should at least check that it wasn't disposed after acquiring the lock
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 added the disposal check after the lock (thanks). I'll add a double-start check.
src/Orleans/Core/ClusterClient.cs
Outdated
{ | ||
get { return this.runtimeClient.ClientInvokeCallback; } | ||
set { this.runtimeClient.ClientInvokeCallback = value; } | ||
} |
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.
These seem like initialization concerns, so if moving to a separate builder, they should go there
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.
Done, kept as internal for easier testing.
src/Orleans/Core/ClusterClient.cs
Outdated
public async Task Start() | ||
{ | ||
this.ThrowIfDisposed(); | ||
using (await this.initLock.LockAsync()) |
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.
all awaits in this class should use .ConfigureAwait(false)
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.
You're right, thanks.
src/Orleans/Core/ClusterClient.cs
Outdated
/// <inheritdoc /> | ||
public void Stop() | ||
{ | ||
this.Stop(gracefully: true).Wait(); |
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 one for example would deadlock in some environments if .ConfigureAwait(false) is not used.
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.
fixed
var client = this.GetClient(); | ||
client?.Stop(); | ||
client?.Dispose(); | ||
this.SetClient(null); |
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.
should SetClient always be called even with exceptions? (ie: in a finally block, or moving it before stopping?)
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.
Fixed, thanks.
Great work @ReubenBond! Looking forward to have my tests back on .Net Core :P |
I'll run functionals again before merge |
Ok, functionals look good |
Thanks! Awesome! Reuben, can you merge to vso/master? |
@jdom huge thanks for reviewing this! I'll do the merge (there's a commit needed to avoid breakages) |
What is the best practice to use these As I see, these are not "cheap" calls, is the suggested Should we handle somewhere the |
Are you considering this for the case of grains talking to grains in another cluster? The primary use case here is clients (frontends) connecting to multiple clusters. There the cost is ~the same as it was with GrainClient.Initialize(). |
Yes, I figured out the client story and would like to clarify the grain-to-grain version, maybe the inter-cluster streaming if possible. As I understand this would be useful for connecting Orleans uServices directly. I'm just preparing for a presentation about Orleans's features (covering even the planned features), that's why I'm asking. |
@lmagyar this is not a tested/supported scenario. The |
@ReubenBond - I'm on 1.5.0-rc. is it possible to create IClusterClient inside silo and use it to call the same silo grains and/or subscribe streams? I was hoping to use it in my shared common libs for both the client and the silo (i.e. listening to general events in the system). I've tried but without success. EDIT - it seems that it is working, will further check and update (hard day today :)) |
@shlomiw yes, it is possible. You can have multiple |
@galvesribeiro / @ReubenBond - the To sum-up an example:
I can try to solve it with coordinations mechanisms, but I was wondering if there's a better approach. And it would be best to use the shared IClusterClient. BTW - my previous solution, before 1.5.0, was having different DI to the shared library exposing IGrainFactory, Streams subscriptions etc. For the IGrainFactory inside a silo - I just captured a context of one of the grains. But it is very hacky. What do you think? thanks in advance! |
TL;DR; If you are inside the target silo, there is no point on use When the silo starts up, the In other hand, your client (assuming you are on 1.5.x and using the new non-static client from this PR) will use There is no DI today (yet) on our client side unfortunately... |
@galvesribeiro - I can't figure out how to use |
Sorry @shlomiw. Please look at my updated reply. It is |
Ok, I understand. I hoped I could use IClusterClient to share the code also in the silo. |
Why? The subscriptions are persistent as far as I know. If a grain is deactivated and have an active subscription, it just need to call I would have a generic interface in your case. Something like this:
Both |
@galvesribeiro - first of all - thanks a lot for your quick replies! it's not trivial :)
Again, it would be best if you'd allow Thanks again! |
I would indeed recommend you do not create an IClusterClient to itself. We do have intentions to make this scenario a lot simpler (basically being able to call the silo outside of the context of a grain), but in the meantime you should avoid creating a real client against itself just for this. Also, be aware that what you are doing with the timers is not robust enough to keep the grain activated forever. Since this is just a normal grain registered in the directory, if the silo that owns that partition of the directory dies, then the activation will realize it's no longer in the directory and be deactivated. @ReubenBond is currently looking into improving this for normal grains. |
@jdom - you're completly right! I have to use a local grain since it's needed per Silo. StatelessWorker is a good idea. |
@ReubenBond - maybe #3362 will solve my above issue and be the elegant solution I was hoping for. To share a common infra code with IClusterClient also for the silo process. |
This PR implements the majority of the remaining items in #467. With this PR merged, an end-user can
The core abstraction which this introduces is
IClusterClient
, which implementsIGrainFactory
. It is implemented by the public classClusterClient
.ClusterClient
has staticCreate
methods which match the existingGrainClient.Initialize
methods with the major difference that they create and do not start the client. A client is started via a call toStart
, which is asynchronous.This does not implement support for automatically binding
GrainReference
s to their originating cluster, but there is anIGrainFactory.BindGrainReference(ref)
method for that purpose - so users aren't completely stuck. The plan is to implement those kinds of features later on, probably leveraging theclusterId
concept from GeoClusters.Calling from one grain into multiple other clusters:
Similarly, for standalone clients: