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

Silo Metadata and Placement Filtering #9271

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

rkargMsft
Copy link
Contributor

@rkargMsft rkargMsft commented Dec 19, 2024

This PR has two main features:

  1. Placement Filter support: Adds ability to annotate grains with filters that can examine and manipulate the set of candidate silos that are passed to the Placement implementation
  2. Silo Metadata: Ability to add (string, string) values to a silo that all silos in the cluster have access to. This aims to address [Proposal] Support defining Silo metadata #8189

Additionally, there are specific implementations of Placement Filtering using the Silo Metadata to specify either required keys that must match, or an ordered set of preferred keys to use for filtering down placement candidates.

  • Add tests
  • Create documentation
    • Silo Metadata
    • Placement Filtering
    • Matching Metadata Placement Filtering
Microsoft Reviewers: Open in CodeFlow

Still requires tests
@rkargMsft
Copy link
Contributor Author

Question: Why is metadata handled separate from the existing membership table as suggested in #8189?
Answer: There is no existing general storage on MembershipEntry and adding a new field would require every Clustering provider (in core, Contrib, and any non-public implementations) to update implementations to store and retrieve a new field for metadata. There isn't a good way to advertise optional features on a clustering provider to validate at startup so we'd be left with either runtime exceptions or with unexpected behavior (silo metadata would get configured but not actually get stored and be available for other silos to read).
Because of that, a separate mechanism is used to transport metadata (GrainServices). This could technically be made a bit more efficient by piggy backing on the existing membership gossip exchange, but given that it's a one-time pull per silo, it's quite likely that the current performance will work for all practical use cases. If there is a scale where even this once-per-silo pull is unacceptable, the current implementation can adjust its internals to us gossip without needing to change the interface exposed to consumers.

@rkargMsft
Copy link
Contributor Author

Question: Why is this implemented in core instead of as a separate package?
Answer: There are several internal types that would be required to be public in order to make this work as a separate package so that approach was abandoned.
Additionally, having access to internal classes like MembershipTableManager makes the process of keeping the local cache of Silo Metadata more efficient since there isn't just blind polling of silo membership as would be required with a separate package.

@miguelhasse
Copy link

miguelhasse commented Dec 28, 2024

@rkargMsft this is something I really need. When can we expect this PR to move from draft? Thanks so much.

@rkargMsft
Copy link
Contributor Author

When I’m back next week I’ll be working on the documentation to get this out of draft. I expect that others will be more available for feedback on the PR in the new year, too.

@rkargMsft
Copy link
Contributor Author

Ordering of filters (through a parameter on the attribute) came up as being necessary as the documentation was being written. The order the attributes are applied in source is not necessarily the same order that they show up when querying metadata so explicit ordering is necessary.

Getting the ordering support implemented atm.

@miguelhasse
Copy link

@rkargMsft I would suggest moving the following into assembly Orleans.Core.Abstractions:

  • PlacementFilterAttribute
  • PlacementFilterStrategy

And eventually move specific implementations for PreferredMatchSiloMetadataPlacementFilter and RequiredMatchSiloMetadataPlacementFilter into a separate assembly (eg: Orleans.Placement.Filtering).

@ReubenBond
Copy link
Member

@rkargMsft I would suggest moving the following into assembly Orleans.Core.Abstractions:

  • PlacementFilterAttribute
  • PlacementFilterStrategy

I agree with this comment

And eventually move specific implementations for PreferredMatchSiloMetadataPlacementFilter and RequiredMatchSiloMetadataPlacementFilter into a separate assembly (eg: Orleans.Placement.Filtering).

We can consider this, but I'm not in favor of fine-grained assembly/package model since it tends to degrade the user experience

@rkargMsft rkargMsft force-pushed the silo-metadata-and-placement-filtering branch from e1ea897 to 4fae612 Compare January 8, 2025 16:25
rkargMsft and others added 2 commits January 8, 2025 08:44
Orleans.Core.Abstractions:
- PlacementFilterAttribute
- PlacementFilterStrategy

Orelans.Core:
- IPlacementFilterDirector
- PlacementFilterExtensions
@rkargMsft rkargMsft marked this pull request as ready for review January 8, 2025 20:17
@rkargMsft
Copy link
Contributor Author

Documentation PR up. Ready for review.

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 this pull request may close these issues.

3 participants