Skip to content

Conversation

@ReubenBond
Copy link
Member

@ReubenBond ReubenBond commented Jan 15, 2026

Microsoft Reviewers: Open in CodeFlow

Copy link
Contributor

Copilot AI left a 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 DelegatingRemoteGrainDirectory infrastructure 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 UseTestClusterGrainDirectory configuration 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);
Copy link

Copilot AI Jan 15, 2026

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.

Suggested change
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}}'.");
}

Copilot uses AI. Check for mistakes.
@ReubenBond ReubenBond marked this pull request as draft January 20, 2026 22:01
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.

1 participant