Ability to specify interleaving per message type #2246

yevhen · 2016-10-01T18:41:05Z

Hi guys!

As you know Orleankka provides a functional message-based style API for Orleans and all grains have a uniform interface like:

interface IActor
{
    Task<object> Receive(object msg);
}

To support some advanced scenarios like ability to specify message interleaving in a more granular way (per-message type) which is incredibly valuable feature we extended this interface with additional method:

interface IActor
{
    Task<object> Receive(object msg);
    [AlwaysInterleave] Task<object> ReceiveReentrant(object msg);
}

So when sending a message via actor reference we were selecting one method ot the other depending on the configuration for this message/actor type.

I'm preparing next big and updated release (1.0, ya finally!) and most anticipated feature is to be able to send messages from clients without need to reference server-side libraries (containing actor definitions). Basically, proper client-server separation.

Unfortunately, current implementation of per-message reentrancy requires to have that information on client (so that we can choose either Receive or ReceiveReentrant). Which is a leak of server-side knowledge and is very illogical.

I haven't found any other way then to extend implementation upstream. It's just a rough sketch not a final code, something to discuss further. I will highly appreciate you help on this issue. Please advice, mesage reentrancy is very important (and used) feature in Orleankka and now this stops me from releasing 1.0 😞

sergeybykov · 2016-10-01T21:12:11Z

What do you think would be the ideal way to specify per-message reentrancy?

We already have InvokeMethodOptions enum with AlwaysInterleave in it, and we could easily add another value to it if necessary. GrainReference.InvokeMethodAsync<T> already takes a value for InvokeMethodOptions options. We just don't expose it in codegen. It should be relatively easy to introduce a per method / per interface attribute that would instruct codegen to add an additional optional parameter, e.g. a bool, that would get translated into a value of InvokeMethodOptions passed down to GrainReference.InvokeMethodAsync<T>.

Alternatively, we could use RequestContext for specifying the reentrancy flag, but IMO that would be ugly.

Are there other options?

yevhen · 2016-10-01T22:10:47Z

Any implementation which doesn't require clients (senders) to know about which message should be sent as reentrant. AFAIU both of suggestions (per method / per interface attribute and `RequestContext') doesn't fit that requirement.

Also, those won't work with F#. Discriminated unions require special handling due to how cases are implemented internally (as static properties!). That's why I've added a function callback,. so I can install a different implementation for C#/F# respectively.

yevhen · 2016-10-01T22:34:06Z

I should probably add some details about upcoming 1.0 client-server implementation in Orleankka.

Previously, we were having fixed (static) set of grain implementations and interfaces covering all possible grain configurations (one for worker, one for actor with random placement, with activation count placement, etc) and acting as generic hosts. The actual type of the actor (in the form of its type code) that need to be instantiated had been sent with every request.

Now, in 1.0 we got rid of this in favor of pre-generating (using Roslyn) grain interfaces dynamically. Since, all interfaces have exactly the same shape - this was only about generating interfaces with exactly the same type name. On server-side, we generate grain interface/implementation and since we have all required information there, we amend the auto generated grain implementation with all neccessary attributes, such as StatelessWorker, Placement, etc. For client-side - it's enough to just generate an interface with exactly the same name as we did for server-side, for Orleans to correctly bind it to server-side implementation. So there is no need to share exactly the same dll with interfaces. It's just enough for client to know server-side actor's type to be able to send him messages.

I don't (and shouldn't) have any server-side information on client. I gues this rules out RequestContext and other suggestions. Plus, the added weirdness of handling F# properly doesn't leave any other choice then to add function callback.

sergeybykov · 2016-10-01T22:36:14Z

Any implementation which doesn't require clients (senders) to know about which message should be sent as reentrant.

What criteria would be used to decide whether to interleave a particular request? Argument type? An expression calculated over argument(s)?

You mentioned in the code that you envision this per type predicate to be installed by a boorstrap provider. Do you see a problem with defining such a predicate as part of the grain class, so that it could be auto-discovered and installed without the need for a bootstrap provider?

sergeybykov · 2016-10-01T22:36:48Z

Or am I completely missing the point?

yevhen · 2016-10-01T22:42:57Z

What criteria would be used to decide whether to interleave a particular request? Argument type? An expression calculated over argument(s)?

It's argument type for both C# and F#. Thus, there is a some weirdness wrt to how properly calculate the argument type when argument is F#'s Discriminated Union.

As F# is functional the callback style looks more idiomatic and will cover both C# and F#. In F# this was previously implemented as conventional static function on actor's type. For C# - this was supported via [ReentrantAttribute(Type msgType)] which you can put on actor class.

yevhen · 2016-10-01T22:56:51Z

Do you see a problem with defining such a predicate as part of the grain class, so that it could be auto-discovered and installed without the need for a bootstrap provider?

No, ok for me. I just don't know how to add this to Orleans so it doesn't look innocent to its core design.

yevhen · 2016-10-01T23:11:11Z

I can imagine something similar to how stream filters are defined now, like putting on the method:

[AlwaysInterleave("SomeClass.SomeStaticCallbackMethod")]
Task SomeMethod(object msg)

Where SomeClass.SomeStaticCallbackMethod points to method having following signature:

bool Predicate(Type grainType, object msg);

sergeybykov · 2016-10-02T01:08:37Z

bool Predicate(Type grainType, object msg);

Shouldn't it rather be
bool Predicate(TypeInfo grainType, InvokeMethodRequest request);?

What about F#? Will such approach work for it?

yevhen · 2016-10-02T07:25:15Z

Yes, it will

ReubenBond · 2016-10-02T07:32:27Z

If we could work this into grain-level or silo-level interceptors, would that work for you? I'm not sure that it can be done cleanly

yevhen · 2016-10-02T07:47:51Z

Interceptor would be fine. Anything that calls my predicate with grain type and method argument info will do.

sergeybykov · 2016-10-02T19:49:39Z

Silo wide interceptors require a bootstrap provider or some other way to install them globally. I think we better avoid such a burden here if we can. Hence, grain level interceptors seem like a more convenient pattern to me. However, if you envisions no grain class specific logic in the predicate, then defining a per class predicate may become a burden.

We could add a method to IGrainInvokeInterceptor or introduce a separate interface. IGrainReentrancyOverride?

public interface IGrainReentrancyOverride
{
    bool CanInterleaveRequest(TypeInfo grainType, InvokeMethodRequest request);
}

The scheduler will likely have to treat grain classes with a reentrancy overrides as reentrant (they are handled differently today) but interleave requests only when CanInterleaveRequest() returns true. Not sure off the top of my head how difficult such a change to the scheduler will be. @gabikliot might have a better idea. This may be another argument for per grain class predicates, so that the scheduler doesn't have to treat all grain classes as reentrant if a per silo predicate is installed.

yevhen · 2016-10-02T20:42:58Z

Sounds complicated to me but you know better how to make it in a most lean and logical way. I'm ok with grain level interceptors if they can amend how the message is delievered (interleaved or not)

sergeybykov · 2016-10-02T21:38:13Z

I don't know if I know better. :-) Just throwing some thoughts for other people to weigh in.

sergeybykov · 2016-10-05T14:36:35Z

Any other opinions here? @ReubenBond, what do you think? @jdom?

We need to unblock @yevhen.

ReubenBond · 2016-10-05T19:16:39Z

Whatever design we adopt, it cannot break the single-threaded invariant.

If we are going to add a new method/interface, are there any other provisions we'd like in that interface? Just to make sure that we're not making adding a specific feature when we could solve this with a more general approach which might result in a less complicated API in the long run. As an example, we're talking about classifying a message as being AlwaysInterleave or not, but messages can also be ReadOnly & Unordered.

I think my comment about using grain-level interceptors is silly, since it executes too late.

sergeybykov · 2016-10-11T03:54:56Z

Here's a proposal.

Add a

bool CanInterleaveRequest(InvokeMethodRequest request);

method to GrainTypeData class. GrainTypeData in its constructor will find if exist all methods of the grain class marked with the [Interleave("SomeClass.SomeStaticCallbackMethod")] attributes and build a method ID->callback map. Can put Reflection-related methods to TypeUtils.

Add a reference to GrainTypeData to ActivationData, and expose GrainTypeData.CanInterleaveRequest(InvokeMethodRequest request) via it.
Add to Dispatcher.CanInterleave another || with a call to targetActivation.CanInterleaveRequest.

Will this be sufficient? Did I miss something?

gabikliot · 2016-10-11T05:41:45Z

Sorry, I am not fully following why this is needed. Can u please briefly summarize what will this achieve?

sergeybykov · 2016-10-11T14:37:09Z

@gabikliot You mean the whole purpose of the change?

In Orleankka, @yevhen implements the Erlang/Akka style model with a single Receive(object message) remote method where different message types conceptually map to methods of a grain interface in the traditional approach but the "dispatching" happens within Receive.

Now for different message types he needs to have different reentrancy rules, as if for different methods in a grain interface. He wants to do that dynamically via a callback. We could consider expanding the scope of this change, so that [Interleave] would also be applicable to traditional grain methods.

@yevhen, is this an accurate description?
@gabikliot, does it make sense now?

yevhen · 2016-10-11T16:13:03Z

@yevhen, is this an accurate description?

absolutely.

gabikliot · 2016-10-11T17:09:52Z

Sergey, yes I meant the whole purpose.
i did not understand the 2nd paragraph of your description. What callback? Why do we need to Change dispatcher? Always interleave is already applicable to regular grain calls, isn't it? Or is it internal attribute?

sergeybykov · 2016-10-11T17:16:55Z

@yevhen has a single method that we wants to interleave for some requests and not to interleave for others. The decision would be made based on the argument types and potentially other data in the request. Hence, a static [Interleave] attribute won't be enough, as the interleaving decision will have to be made on a per-request basis. That's why he needs a callback - to make the interleaving decisions for each request. The change to Dispatcher is needed to invoke this callback (if it's defined) in addition to the reentrancy checks it already performs in CanInterleave.

gabikliot · 2016-10-11T18:26:48Z

Ohh, I think I understand now.

So alternatively, he could have implemented the queuing himself, in his dispatch logic in the silo interceptor. In this approach, from Orleans perspective all calls can interleave (there will be a single call 'DO' with AlwaysInterleave) and @yevhen will decide himself what should be queued and queue it himself in his logic, and what can be invoked directly., and dispatch the queued one when the previous done, etc....
But, that means a lot of logic duplication with the already existing Orleans logic in the Dispatcher, so instead of implementing it all, @yevhen wants the Dispatcher to be able to call into his callback. Dispatcher will be the execution/queueing/dispatching mechanism, while his code can provide dynamic policy.

Is that a correct description?

yevhen · 2016-10-11T20:45:24Z

@gabikliot ye, close to. Previously, I have these two methods:

interface IActor : IGrainWithStringKey 
{
    Task<object> Receive(object msg);
    [AlwaysInterleave] Task<object> ReceiveReentrant(object msg);
}

so grains we not Reentrant in general and when message was being sent through my grain reference wrapper, I was inspecting the type of the message and depending on configuration one method or the other was chosen.

Unfortunately, when sending message from client to server with that approach it means that server configuration (which type of message need to be sent via interleaved method) need to be available on the client, which doesn't seem right.

Also, what's more important - it doesn't work with streams 😞

yevhen · 2016-10-11T20:49:00Z

I kinda solved it by marking all actors as Reentrant and then using AsyncSerialExecutor to execute non-interleaved calls serially but it adds additional overhead to every actor and call. So it would be really cool if such support is available out-of-the-box from underlying framework. Will make it 100% friendly for message-based style of communication.

sergeybykov · 2016-10-11T21:37:57Z

@yevhen

Also, what's more important - it doesn't work with streams 😞

What's the problem with streams?

@gabikliot

But, that means a lot of logic duplication with the already existing Orleans logic in the Dispatcher, so instead of implementing it all, @yevhen wants the Dispatcher to be able to call into his callback. Dispatcher will be the execution/queueing/dispatching mechanism, while his code can provide dynamic policy.

Yes, exactly - the callback would supply a kind of a dynamic reentrancy policy.

Do you see any holes in my proposal above? I assume no changes in the scheduler will be required, as we'll leverage the Dispatcher's reentrancy logic, but I'm not 100% sure I'm not missing something.

jdom

In general I like the approach, but as I mentioned, I'd combine it with some way of statically defining the message interleaving, instead of having to dig into all GrainTypeData objects when starting up to inject this functionality.

jdom · 2016-10-11T22:06:10Z

src/OrleansRuntime/GrainTypeManager/GrainTypeData.cs

+
+        public bool IsMessageInterleaves(Message message)
+        {
+            return IsReentrant || IsMessageInterleavesPredicate(message.BodyObject.GetType());


I take it that in Orleankka you do not use the InvokeMethodRequest as the BodyObject, otherwise this wouldn't give you what you want

that's just spike :)

Ye, I need a method request argument.

jdom · 2016-10-11T22:09:04Z

src/OrleansRuntime/GrainTypeManager/GrainTypeData.cs

@@ -127,5 +128,19 @@ internal static MultiClusterRegistrationStrategy GetMultiClusterRegistrationStra
                            grainClass.Name));
            }
        }
+
+        // used by Orleankka's bootstrapper
+        internal void SetMessageAlwaysInterleaves(Func<object, bool> predicate)


it would be good to combine this with @sergeybykov's suggestion of being able to define it statically, with an attribute or something on the grain implementation or interface, or even on the argument type as defined in the interfaces that the silo uses. I would prefer on the grain type itself, since the argument type could be re-used across grain implementations, and each have different interleaving implications, or even different grain implementations for the same grain interface might have different needs.

grain implementation or interface

implementation. I think interface is not a good candidate since it's reentrancy is an implementation detail.

yevhen · 2016-10-12T04:00:11Z

What's the problem with streams?

In current design grains are not marked as Reentrant so stream subscriptions inside actor are not reentrant as well. If I mark it as Reentrant I'll loose ability to specify reentrancy in a granular way.

jdom · 2016-10-24T21:44:46Z

Be careful that if you want to re-open the PR eventually, you have to re-open with exactly the same original commits as before closing, and only after that rebase the branch. Otherwise GitHub just thinks they are entirely different branches and want allow you to re-open the same PR.

jdom · 2016-10-24T21:51:05Z

btw, my preference would also be MayInterleave

yevhen · 2016-10-24T22:48:07Z

I think it's easier to create new PR.

jdom · 2016-10-24T23:04:52Z

As you prefer. Nevertheless if it was my choice, I'd prefer to re-open this one, as all the discussions happened here. Basically just do the rebase (or re-do) on a separate branch, reopen this, and then force push to this branch

yevhen · 2016-10-25T09:00:27Z

Ok, figured out what was wrong.

yevhen · 2016-10-25T09:13:57Z

It would be better to squash changes but for now could be seen here https://github.com/dotnet/orleans/pull/2246/files

I think CanInterleave method in GrainTypeData need to be called MayInterleave and probably the field as well to be consistent with MayInterleave attribute name.

sergeybykov · 2016-10-26T16:36:58Z

Looks good to me as is. I kicked off a load test just to confirm there is no impact on perf. But I can't imagine that. Will merge after that unless I hear objections.

I think CanInterleave method in GrainTypeData need to be called MayInterleave and probably the field as well to be consistent with MayInterleave attribute name.

Did you rather mean Catalog.CanInterleave? I'm fine either way with a slight preference for CanInterleave because Catalog gives a definitive answer to Dispatcher whether a specific message can interleave or not.

jdom · 2016-10-26T16:55:03Z

test/TestGrains/ReentrantGrain.cs

@@ -68,6 +71,84 @@ public Task SetSelf(INonReentrantGrain self)
        }
    }

+    [MayInterleave("MayInterleave")]


Does [MayInterleave(nameof(MayInterleave)] compile at this scope (or call the method MayInterleavePredicate to make it more explicit that it has hothing to do with the attribute name)? If so, that would be preferred, as people might look at this test to get a sense of how the feature works

sergeybykov · 2016-10-27T04:36:56Z

Perf test looks good.
Thank you, @yevhen!

yevhen · 2016-10-27T12:03:30Z

Cool, thanks! When to expect next minor release? ))

sergeybykov · 2016-10-27T17:26:03Z

1.3.1. is in the works. See #2350.

@gabikliot

* Ability to specify interleaving per message type * Rename methods as per @gabikliot review comments * Add tests for message interleaving predicate (false/true) * Add tests for stream delivery and when predicate throws * Introduce attribute. Review fixes * Make naming consistent * Use nameof instead of string

dnfclas added the cla-already-signed label Oct 1, 2016

jdom reviewed Oct 11, 2016

View reviewed changes

yevhen force-pushed the always-interleave-per-message-type branch from ee8ad76 to 4d7ba57 Compare October 24, 2016 21:34

yevhen reopened this Oct 25, 2016

dnfclas added the cla-already-signed label Oct 25, 2016

yevhen force-pushed the always-interleave-per-message-type branch from 521ffa8 to 4269cb0 Compare October 25, 2016 09:09

sergeybykov added the testing label Oct 26, 2016

yevhen added 6 commits October 26, 2016 19:36

Ability to specify interleaving per message type

6e1e4d2

Rename methods as per @gabikliot review comments

a3704df

Add tests for message interleaving predicate (false/true)

4313c1e

Add tests for stream delivery and when predicate throws

1c34ad2

Introduce attribute. Review fixes

62a51d1

Make naming consistent

b4105c4

yevhen force-pushed the always-interleave-per-message-type branch from 4269cb0 to b4105c4 Compare October 26, 2016 16:36

jdom approved these changes Oct 26, 2016

View reviewed changes

Use nameof instead of string

ce110d8

sergeybykov merged commit 491fbc8 into dotnet:master Oct 27, 2016

jason-bragg mentioned this pull request Jan 24, 2019

Orleans AlwaysInterleave question OrleansContrib/Orleankka#150

Closed

github-actions bot locked and limited conversation to collaborators Dec 12, 2023

Ability to specify interleaving per message type #2246

Ability to specify interleaving per message type #2246

Conversation

yevhen commented Oct 1, 2016

sergeybykov commented Oct 1, 2016

yevhen commented Oct 1, 2016 • edited

yevhen commented Oct 1, 2016 • edited

sergeybykov commented Oct 1, 2016

sergeybykov commented Oct 1, 2016

yevhen commented Oct 1, 2016

yevhen commented Oct 1, 2016

yevhen commented Oct 1, 2016

sergeybykov commented Oct 2, 2016

yevhen commented Oct 2, 2016

ReubenBond commented Oct 2, 2016

yevhen commented Oct 2, 2016 • edited

sergeybykov commented Oct 2, 2016

yevhen commented Oct 2, 2016

sergeybykov commented Oct 2, 2016

sergeybykov commented Oct 5, 2016

ReubenBond commented Oct 5, 2016

sergeybykov commented Oct 11, 2016

gabikliot commented Oct 11, 2016

sergeybykov commented Oct 11, 2016

yevhen commented Oct 11, 2016

gabikliot commented Oct 11, 2016

sergeybykov commented Oct 11, 2016 • edited

gabikliot commented Oct 11, 2016

yevhen commented Oct 11, 2016

yevhen commented Oct 11, 2016 • edited

sergeybykov commented Oct 11, 2016

jdom left a comment

Choose a reason for hiding this comment

jdom Oct 11, 2016 • edited

Choose a reason for hiding this comment

yevhen Oct 12, 2016

Choose a reason for hiding this comment

jdom Oct 11, 2016 • edited

Choose a reason for hiding this comment

yevhen Oct 12, 2016

Choose a reason for hiding this comment

yevhen commented Oct 12, 2016

jdom commented Oct 24, 2016

jdom commented Oct 24, 2016

yevhen commented Oct 24, 2016

jdom commented Oct 24, 2016

yevhen commented Oct 25, 2016 • edited

yevhen commented Oct 25, 2016

sergeybykov commented Oct 26, 2016

jdom Oct 26, 2016

Choose a reason for hiding this comment

sergeybykov commented Oct 27, 2016

yevhen commented Oct 27, 2016

sergeybykov commented Oct 27, 2016

yevhen commented Oct 1, 2016 •

edited

yevhen commented Oct 1, 2016 •

edited

yevhen commented Oct 2, 2016 •

edited

sergeybykov commented Oct 11, 2016 •

edited

yevhen commented Oct 11, 2016 •

edited

jdom Oct 11, 2016 •

edited

jdom Oct 11, 2016 •

edited

yevhen commented Oct 25, 2016 •

edited