dotnet / orleans

Cloud Native application framework for .NET
https://docs.microsoft.com/dotnet/orleans
MIT License
10.06k stars 2.03k forks source link

Codegen does not convert [Unordered] interface attribute into method metadata InvokeMethodOptions.Unordered + Proposed Fix #9149

Open MarkNickeson opened 1 week ago

MarkNickeson commented 1 week ago

In principle, [Unordered] is applied to grain interfaces to instruct gateway connection selection to be round-robin instead of hashed to GrainId. However, the test "TestWithConstantId_ExpectTwo" depicted in this gist demonstrates that round-robin behavior does not occur. Debugging into source, the key finding is that metadata for methods within interfaces decorated with [Unordered] indicates InvokeMethodOptions.None rather than including the required InvokeMethodOptions.Unordered.

A potential solution would require revision to InvokableMethodDescription.cs. However, this is potentially risky given that other important Orleans capabilities such as reminders make use of [Unordered] - the potential for unintended consequences seems high.

To fix the problem, pseudo-code similar to below must be added to the ctor in InvokableMethodDescription.cs:

if containingInterface is decorated with UnorderedAttribute { CustomInitializerMethods.Add(("AddInvokeMethodOptions",InvokeMethodOptions.Unordered)) }

ReubenBond commented 6 days ago

I have a branch fixing most of this, but I think it would be better to deprecate [Unordered] (clearly no one relies on its behavior). [Unordered] is redundant: there were never any ordering guarantees for unawaited calls. [StatelessWorker] is different because it is multi-activation and hence multiple silos can host [StatelessWorker] instances. Still, I'm not sure the behavior is that important: when you call a stateless worker from within a silo you will not get this behavior. There is code implementing it for external ClusterClients, but even that was not being leveraged and we have tests asserting the existing behavior.

EDIT: Could you elaborate on your original use case, @MarkNickeson? I want to understand the ask better

MarkNickeson commented 6 days ago

Thanks, Reuben! I appreciate your feedback and fully understand the Occam's razor approach. Below is a first cut at describing the use case:

Use case

Reasoning

Alternatives considered and why deemed undesirable 1) Ingress grain with random ID - high activation freq but single use leads to lots of churn 2) Well known grain id w/ re-reentrancy - one silo becomes hot spot 3) Locator pattern (grain service activates per silo worker(s), registers centrally - suffers from 2 OR requires client-side complexity 4) Streams - do not migrate well between cluster versions. Using deployment approach based on Orleans versioning that is tested, enables "version-1" quiescing and migrates well to "latest" 5) Briefly looked at GrainService as entry point - not accessible via external client afaict

ReubenBond commented 6 days ago

Thanks for the context! We could make this work for stateless workers specifically and mark the attribute as obsolete. How does that sound to you?

MarkNickeson commented 6 days ago

Sounds good to me! I am curious how it will be achieved without an attribute or marker interface given that [StatelessWorker] is an implementation aspect. But I'm here to learn and look forward to eventually it trying out and putting it to work!

Thanks again!

ReubenBond commented 6 days ago

The gist of it is this: