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

Add Cacheable Entity #14337

Closed
wants to merge 9 commits into from
Closed

Add Cacheable Entity #14337

wants to merge 9 commits into from

Conversation

MikeAlhayek
Copy link
Member

@MikeAlhayek MikeAlhayek commented Sep 14, 2023

The interface ICacheableEntity provides a way to cache properties in IEntity. This will improve the speed of retrieving the settings from SiteService.

@sebastienros brought up this issue during the call today.

@jtkech thoughts on this approach?

In this current approach, a new ICacheableEntity interface was added. The idea is to make IEntity cacheable when needed. If you need (you should in most cases) IEntity, then implement ICacheableEntity instead of IEntity or inherit from CacheableEntity instead of Entity.

If we want to make IEntity cacheable all the time, we can simply drop the new ICacheableEntity interface, and move it's methods to IEntity instead. I just don't know if using cache for everything by default is a good idea. Maybe it is, I am just not very convinced. if @sebastienros and @jtkech think it's appropriate, I can simplify by removing ICacheableEntity.

@MikeAlhayek
Copy link
Member Author

MikeAlhayek commented Sep 14, 2023

@sebastienros I am moving the discussion here. In your comment you stated the following

Wouldn't it be simpler to just add methods to IEntity and have existing classes implement it?
But I will think more about this new interface, maybe it's the name that bothers me.

You may be able to suggest better one. Ideas?

The problem adding a new property on IEnitity is that we may need an entity that isn't cacheable. I don't know if we need an IEnitity that does not use cache. we already have any classes that implement the IEntity like DocumentEntity and Entity.

But if you think IEnitity should be cacheable everywhere, I can move the 3 methods into IEntity

void Set(string key, object value);
void Forget(string key);
object Get(string key);

@jtkech
Copy link
Member

jtkech commented Sep 14, 2023

@jtkech thoughts on this approach?

Okay, will look at it when I will have time.


namespace OrchardCore.Settings
{
// When updating class also update SiteSettingsDeploymentSource and SettingsStep.
public class SiteSettings : DocumentEntity, ISite
public class SiteSettings : DocumentEntity, ICacheableEntity, ISite
Copy link
Member

Choose a reason for hiding this comment

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

I think you need to let DocumentEntity implements ICacheableEntity instead, so all the implementation will be no long exist here

Copy link
Member Author

@MikeAlhayek MikeAlhayek Sep 15, 2023

Choose a reason for hiding this comment

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

Not sure we want that behavior applied everywhere. If that is the cache, we can just remove ICacheableEntity and move the new methods into IEntity instead. Look at the notes in the description of the PR.

@hishamco
Copy link
Member

@MikeAlhayek, please check my changes

Copy link
Member Author

@MikeAlhayek MikeAlhayek left a comment

Choose a reason for hiding this comment

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

@hishamco not a fan of the changes.

@hishamco
Copy link
Member

Check my comment, otherwise I will revert my changes :)

@MikeAlhayek
Copy link
Member Author

@hishamco I left you comments on the changes you made please visit them so we can finalize the PR.

@hishamco
Copy link
Member

I might let @jtkech review this PR too if he has a time

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