Skip to content
This repository has been archived by the owner on Jun 19, 2020. It is now read-only.

(FACT-2481) Fix caching #399

Merged
merged 8 commits into from
Mar 24, 2020
Merged

(FACT-2481) Fix caching #399

merged 8 commits into from
Mar 24, 2020

Conversation

BogdanIrimie
Copy link
Contributor

No description provided.

@BogdanIrimie BogdanIrimie marked this pull request as ready for review March 23, 2020 15:06
@BogdanIrimie BogdanIrimie requested review from a team March 23, 2020 15:06
Copy link
Contributor

@Filipovici-Andrei Filipovici-Andrei left a comment

Choose a reason for hiding this comment

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

I think we are going in the wrong direction with the @fact_list and @semaphore. They shouldn't be tampered with in the resolvers. They should exist only in the BaseResolver. BaseResolver should be like a template, for what resolvers should do, and each resolver would implement just the post_resolve method. Caching, logging and synchronising should be dealt with in just ONE PLACE, in the BaseResolver.

@@ -30,14 +30,13 @@ class SystemProfiler < BaseResolver

@log = Facter::Log.new(self)
@semaphore = Mutex.new
@fact_list = {}
Copy link
Contributor

Choose a reason for hiding this comment

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

In other resolvers you have @fact_list ||= {}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As @fact_list is initialise on class creation, there is no need for ||=

@@ -6,7 +6,7 @@ module Macosx
class Processors < BaseResolver
@log = Facter::Log.new(self)
@semaphore = Mutex.new
@fact_list ||= {}
@fact_list = {}
Copy link
Contributor

Choose a reason for hiding this comment

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

In other resolvers you have @fact_list ||= {}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As @fact_list is initialise on class creation, there is no need for ||=

@@ -4,12 +4,13 @@ module Facter
module Resolvers
class NetworkingLinux < BaseResolver
@semaphore = Mutex.new
@fact_list ||= {}
Copy link
Contributor

Choose a reason for hiding this comment

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

should this be @fact_list = {} as in other resolvers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes

@BogdanIrimie
Copy link
Contributor Author

@Filipovici-Andrei @semaphore serialize access to one resolver, it should be declared and used at a resolver level, otherwise it would serialize access to all resolvers! Caching is done at the resolver level and should be kept at this level because serialisation is done at this level. The logger can be moved to base resolver.

@BogdanIrimie BogdanIrimie merged commit e98a221 into master Mar 24, 2020
@oanatmaria oanatmaria deleted the FACT-2481 branch April 2, 2020 11:44
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.

3 participants