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

refactor: remove duplicate cache service initialization #3199

Merged
merged 2 commits into from
Apr 14, 2023

Conversation

scopsy
Copy link
Contributor

@scopsy scopsy commented Apr 14, 2023

What change does this PR introduce?

Previously the caching service was initialized 3 times because it was injected another un-needed time.

Why was this change needed?

I'm concerned that we have a split connection for some of the redis calls.

Other information (Screenshots)

@scopsy
Copy link
Contributor Author

scopsy commented Apr 14, 2023

@p-fernandez is there any specific reason for the previous implementation? Or is it okey if we change it to this?

@p-fernandez
Copy link
Contributor

@p-fernandez is there any specific reason for the previous implementation? Or is it okey if we change it to this?

The current implementation was to follow our instantiation pattern and the way I quickly found the less times the services were initialised.
What I do not understand from the PR is that there is no Cache Service changes related even if that is the goal of the PR.

@@ -102,7 +102,6 @@ const distributedLockService = {
};

const PROVIDERS = [
inMemoryProviderService,
Copy link
Contributor

Choose a reason for hiding this comment

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

@scopsy @p-fernandez
I'm wondering why can't we just use the DI mechanism to inject the InMemoryProviderService to the CacheService and DistributedLockService?... we have here above the provider object and we do call useFactory two times which creates two instances of InMemoryProviderService...

@LetItRock
Copy link
Contributor

@p-fernandez is there any specific reason for the previous implementation? Or is it okey if we change it to this?

The current implementation was to follow our instantiation pattern and the way I quickly found the less times the services were initialised. What I do not understand from the PR is that there is no Cache Service changes related even if that is the goal of the PR.

the CacheService is initialized in the shared module where we do pass the instance on InMemoryProviderService

@p-fernandez
Copy link
Contributor

p-fernandez commented Apr 14, 2023

the CacheService is initialized in the shared module where we do pass the instance on InMemoryProviderService

I might miss any nuance but I do not think these changes would remove the double initialisation for the CacheService. Unless the goal of the task is to reduce the initialisations of the InMemoryProviderService to create less instances of the ioredis client downstream, that indeed is happening.

@LetItRock
Copy link
Contributor

the CacheService is initialized in the shared module where we do pass the instance on InMemoryProviderService

I might miss any nuance but I do not think these changes would remove the double initialisation for the CacheService. Unless the goal of the task is to reduce the initialisations of the InMemoryProviderService to create less instances of the ioredis client downstream, that indeed is happening.

we will still create two instances of InMemoryProviderService, check this comment: #3199 (comment)

@scopsy
Copy link
Contributor Author

scopsy commented Apr 14, 2023

Yes exactly what @p-fernandez, just previously there was a third instance of the cache service without a really need for it, since the 2 we have right now are the actually needed ones.

@scopsy scopsy merged commit ca2ba83 into next Apr 14, 2023
11 checks passed
@scopsy scopsy deleted the remove-un-needed-caching-service branch April 14, 2023 14:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants