-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Remove [Experimental] from AddDistributedGrainDirectory
#9882
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
base: main
Are you sure you want to change the base?
Remove [Experimental] from AddDistributedGrainDirectory
#9882
Conversation
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.
Pull request overview
This pull request removes the [Experimental] attribute from AddDistributedGrainDirectory, promoting the Distributed Grain Directory feature from experimental to stable. The PR includes comprehensive migration tests to ensure rolling upgrades from LocalGrainDirectory to DistributedGrainDirectory work correctly.
Changes:
- Removes
[Experimental("ORLEANSEXP003")]attribute and associated pragma warnings across test and playground files - Adds
DelegatingRemoteGrainDirectoryinfrastructure to support rolling upgrades from old to new directory implementations - Introduces comprehensive migration tests (
GrainDirectoryMigrationTests) with ~700 lines validating availability during migration and consistency after migration - Refactors service registrations to support both directory implementations running simultaneously during upgrades
- Adds
UseTestClusterGrainDirectoryconfiguration option to control whether test clusters use shared in-memory grain directories
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/Orleans.Runtime/Hosting/CoreHostingExtensions.cs |
Removes [Experimental] attribute and enhances AddDistributedGrainDirectory with logic to remove LocalGrainDirectory registrations and register delegating services for rolling upgrades |
src/Orleans.Runtime/Hosting/DefaultSiloServices.cs |
Registers DistributedGrainDirectory components on all silos and changes default IGrainLocator to use DhtGrainLocator |
src/Orleans.Runtime/GrainDirectory/DelegatingRemoteGrainDirectory.cs |
New file implementing system targets that delegate old-style IRemoteGrainDirectory calls to DistributedGrainDirectory for rolling upgrade support |
src/Orleans.Runtime/GrainDirectory/LocalGrainDirectory.cs |
Removes Factory dependency and directly instantiates partition |
src/Orleans.Runtime/GrainDirectory/LocalGrainDirectoryPartition.cs |
Changes constructor to accept ILogger instead of ILoggerFactory |
src/Orleans.Runtime/GrainDirectory/GrainDirectoryHandoffManager.cs |
Removes unused Factory parameter |
src/Orleans.Runtime/GrainDirectory/GrainDirectoryPartition.cs |
Fixes indentation and changes log level from Error to Warning for operation invocation errors |
src/Orleans.Runtime/GrainDirectory/GrainLocatorResolver.cs |
Changes DhtGrainLocator to IGrainLocator and marks class as sealed |
src/Orleans.Runtime/GrainDirectory/CachedGrainLocator.cs |
Marks class as sealed |
src/Orleans.Runtime/GrainDirectory/DistributedGrainDirectory.cs |
Moves _localActivations field declaration to fix ordering |
src/Orleans.Runtime/Core/InternalGrainRuntime.cs |
Removes unused ILocalGrainDirectory property |
src/Orleans.Core/Configuration/ServiceCollectionExtensions.cs |
Adds TaggedServiceDescriptor to track implementation types for selective service removal |
src/Orleans.TestingHost/InProcTestClusterOptions.cs |
Changes UseTestClusterMembership visibility to public and adds UseTestClusterGrainDirectory option |
src/Orleans.TestingHost/InProcTestClusterBuilder.cs |
Sets default value for UseTestClusterGrainDirectory to true |
src/Orleans.TestingHost/InProcTestCluster.cs |
Adds validation for UseTestClusterGrainDirectory and conditional grain directory registration |
src/Orleans.TestingHost/ConfigureDistributedGrainDirectory.cs |
Removes pragma warnings for experimental attribute |
test/TesterInternal/GrainDirectory/GrainDirectoryMigrationTests.cs |
New comprehensive test suite (~700 lines) validating rolling upgrade scenarios from LocalGrainDirectory to DistributedGrainDirectory |
test/TesterInternal/GrainDirectory/GrainDirectoryResilienceTests.cs |
Removes pragma warnings |
test/TesterInternal/LocalGrainDirectoryPartitionTests.cs |
Renames class, updates constructor call to use NullLogger, and adds using statement for Abstractions |
test/Tester/LocalhostSiloTests.cs |
Removes pragma warnings |
playground/ChaoticCluster/ChaoticCluster.Silo/SiloBuilderConfigurator.cs |
Removes pragma warnings and fixes formatting |
playground/ActivationSheddingToy/Program.cs |
Removes pragma warnings |
Comments suppressed due to low confidence (2)
src/Orleans.Runtime/Hosting/CoreHostingExtensions.cs:149
- The XML documentation comment still refers to this as 'the experimental distributed grain directory', but the feature is being promoted to stable. Update the documentation to remove 'experimental' and describe it as the distributed grain directory feature.
/// Opts-in to the experimental distributed grain directory.
test/TesterInternal/GrainDirectory/GrainDirectoryMigrationTests.cs:1
- Using
GetHashCode()as a version tag on line 124 in DelegatingRemoteGrainDirectory.cs may not provide stable or meaningful version information. GetHashCode() is not guaranteed to be consistent across process restarts or platforms. Consider using a more stable versioning mechanism or document why this is acceptable for the use case.
#nullable enable
|
|
||
| // Determine if this silo should use DistributedGrainDirectory based on its name | ||
| // Silo_0 and Silo_1 are OLD silos, Silo_2+ are NEW silos | ||
| var siloNumber = int.Parse(options.SiloName.Split('_')[1], CultureInfo.InvariantCulture); |
Copilot
AI
Jan 15, 2026
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.
The silo name parsing logic assumes a specific naming convention (Silo_N) but doesn't validate it. If the silo name doesn't match this format, this will throw an exception. Consider adding validation or using a more robust parsing approach with error handling.
| var siloNumber = int.Parse(options.SiloName.Split('_')[1], CultureInfo.InvariantCulture); | |
| const string siloNamePrefix = "Silo_"; | |
| var siloName = options.SiloName; | |
| if (siloName is null | |
| || !siloName.StartsWith(siloNamePrefix, StringComparison.Ordinal) | |
| || !int.TryParse(siloName.AsSpan(siloNamePrefix.Length), NumberStyles.None, CultureInfo.InvariantCulture, out var siloNumber)) | |
| { | |
| throw new InvalidOperationException($"Unexpected silo name format '{siloName}'. Expected format 'Silo_{{number}}'."); | |
| } |
Microsoft Reviewers: Open in CodeFlow