Skip to content
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

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

Open
MarkNickeson opened this issue Sep 25, 2024 · 5 comments · May be fixed by #9153

Comments

@MarkNickeson
Copy link
Contributor

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
Copy link
Member

ReubenBond commented Sep 25, 2024

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
Copy link
Contributor Author

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

  • Leverage StatelessWorker semantics directly from external client to support high ingress scenario

Reasoning

  • any compatible silo can receive / handle
  • scale ingress via silo count
  • tunable via maxLocalWorkers
  • well known grain id - no coordination required

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

The gist of it is this:

  • When you call IGrainFactory.GetGrain<TInterface>(grainKey), the client needs to know which grain type to map it to (grainId = $"{grainType}/{grainKey}"). To accomplish this, it consults the cluster manifest. Clients and silos synchronize this around so they can maintain a mapping from GrainInterfaceType to GrainType, among other things...
  • Among those other things, the manifest includes properties associated with that grain type, such as how placement works, which implicit stream subscriptions it has, and (potentially) whether or not it's a stateless worker.
  • When creating a GrainReference, the grain factory implementation builds up an object of type GrainReferenceShared, which is created per-GrainType. Here's the ctor to give you an idea of what it provides:
public GrainReferenceShared(
    GrainType grainType,
    GrainInterfaceType grainInterfaceType,
    ushort interfaceVersion,
    IGrainReferenceRuntime runtime,
    InvokeMethodOptions invokeMethodOptions,
    CodecProvider codecProvider,
    CopyContextPool copyContextPool,
    IServiceProvider serviceProvider)
{
    this.GrainType = grainType;
    this.InterfaceType = grainInterfaceType;
    this.Runtime = runtime;
    this.InvokeMethodOptions = invokeMethodOptions;
    this.CodecProvider = codecProvider;
    this.CopyContextPool = copyContextPool;
    this.ServiceProvider = serviceProvider;
    this.InterfaceVersion = interfaceVersion;
}
  • The InvokeMethodOptions from GrainReferenceShared needs to be populated with the Unordered flag during creation and then or'd against the per-method InvokeMethodOptions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants