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

Use GrainDirectoryCacheFactory to construct a IGrainDirectoryCache #8844

Merged
merged 5 commits into from
Mar 7, 2024

Conversation

Mostafa-Goher
Copy link
Contributor

@Mostafa-Goher Mostafa-Goher commented Feb 7, 2024

use GrainDirectoryCacheFactory to construct a IGrainDirectoryCache instead of hardcoded value.
#8843

Microsoft Reviewers: Open in CodeFlow

@Mostafa-Goher
Copy link
Contributor Author

@dotnet-policy-service agree

{
this.grainDirectoryResolver = grainDirectoryResolver;
this.clusterMembershipService = clusterMembershipService;
this.cache = new LRUBasedGrainDirectoryCache(GrainDirectoryOptions.DEFAULT_CACHE_SIZE, GrainDirectoryOptions.DEFAULT_MAXIMUM_CACHE_TTL);
this.cache = GrainDirectoryCacheFactory.CreateGrainDirectoryCache(serviceProvider, grainDirectoryOptions.Value);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue is that this method can return an AdaptiveGrainDirectoryCache, that only make sense when the builtin LocalGrainDirectory is used. It will do weird thing if used here.

But I agree that it would be nicer to be able to inject/customize the cache used here.

Copy link
Contributor Author

@Mostafa-Goher Mostafa-Goher Feb 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, how about if I add a new function in GrainDirectoryCacheFactory to get IGrainDirectoryCache from DI if there is one or return LRUBasedGrainDirectoryCache as a default?
this way we keep the current behavior as is but allow injecting custom IGrainDirectoryCache.
something like:

   public static IGrainDirectoryCache CreateGrainDirectoryCache(IServiceProvider services, GrainDirectoryOptions options)
   {
       var grainDirectoryCache = services.GetService<IGrainDirectoryCache>();
       if (grainDirectoryCache != null)
       {
           return grainDirectoryCache;
       }
       else
       {
           return new LRUBasedGrainDirectoryCache(options.CacheSize, options.MaximumCacheTTL);
       }
   }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@benjaminpetit any feedback about the above?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I was OOF, catching up in things... Yes I think that's better!

Copy link
Contributor Author

@Mostafa-Goher Mostafa-Goher Feb 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@benjaminpetit no worries, welcome back 🙂
I have pushed the new GrainDirectoryCacheFactory function, 'Naming things` strikes again but I did my best to come up with a name and added a summary to explain what it should do, please tell me if the name is not clear or if it can be improved 😉

@ReubenBond ReubenBond changed the title use GrainDirectoryCacheFactory to construct a IGrainDirectoryCache Use GrainDirectoryCacheFactory to construct a IGrainDirectoryCache Feb 22, 2024
@ReubenBond ReubenBond force-pushed the main branch 2 times, most recently from 8924cad to 802ec09 Compare March 5, 2024 17:38
Mostafa-Goher and others added 4 commits March 7, 2024 09:36
…get IGrainDirectoryCache from DI or create the default LRUBasedGrainDirectoryCache.

Update CachedGrainLocator to use GrainDirectoryCacheFactory.CreateCustomGrainDirectoryCache to create inner cache.
@ReubenBond ReubenBond enabled auto-merge (squash) March 7, 2024 17:38
@ReubenBond ReubenBond disabled auto-merge March 7, 2024 17:40
@ReubenBond ReubenBond enabled auto-merge (squash) March 7, 2024 17:40
@ReubenBond ReubenBond merged commit ab644bf into dotnet:main Mar 7, 2024
16 of 19 checks passed
ReubenBond pushed a commit to ReubenBond/orleans that referenced this pull request Mar 7, 2024
…otnet#8844)

---------

Co-authored-by: Mostafa Abdalla <mostafa.abdalla@cm.com>
Co-authored-by: Reuben Bond <203839+ReubenBond@users.noreply.github.com>
(cherry picked from commit ab644bf)
ReubenBond added a commit that referenced this pull request Mar 8, 2024
…8844) (#8898)

---------

Co-authored-by: Mostafa Abdalla <mostafa.abdalla@cm.com>
Co-authored-by: Reuben Bond <203839+ReubenBond@users.noreply.github.com>
(cherry picked from commit ab644bf)

Co-authored-by: Mostafa Goher <Mostafa.Goher@gmail.com>
@github-actions github-actions bot locked and limited conversation to collaborators Apr 7, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants