PR#373: Avoid some unnecessary reloads in the cached functions - fedora-hubs

fedora-hubs

#373 Avoid some unnecessary reloads in the cached functions

Merged 6 years ago by abompard. Opened 6 years ago by abompard.

abompard/fedora-hubs optimize-widget-caching into develop

Add a subclassable function to signal a value should not be cached

Aurélien Bompard • 6 years ago

d2a300e

Add a switch for config-independant cached functions

Aurélien Bompard • 6 years ago

45084e0

Set some defaults for the autodoc Sphinx extension

Aurélien Bompard • 6 years ago

d9cffe7

docs/api/utils.rst

file modified

-5

		`@@ -5,23 +5,18 @@`
		`----------`

		`.. automodule:: hubs.utils.views`
		`- :members:`

		`Caching utils`
		`-------------`

		`.. automodule:: hubs.utils.cache`
		`- :members:`

		`Package database utils`
		`----------------------`

		`.. automodule:: hubs.utils.packages`
		`- :members:`

		`Datagrepper utils`
		`-----------------`

		`.. automodule:: hubs.utils.datagrepper`
		`- :members:`
		`-`

docs/api/widgets.rst

file modified

+3 -12

		`@@ -5,40 +5,31 @@`
		`=======`

		`.. automodule:: hubs.widgets`
		`- :members:`
		`- :show-inheritance:`

		`Widgets registry`
		`----------------`

		`.. automodule:: hubs.widgets.registry`
		`- :members:`
		`- :show-inheritance:`

		`Widget class`
		`------------`

		`.. automodule:: hubs.widgets.base`
		`- :members:`
		`- :show-inheritance:`
		`+ :private-members:`

		`Widget parameter validators`
		`---------------------------`

		`.. automodule:: hubs.widgets.validators`
		`- :members:`
		`- :show-inheritance:`

		`Widget view`
		`-----------`

		`.. automodule:: hubs.widgets.view`
		`- :members:`
		`- :show-inheritance:`
		`+ :private-members:`

		`Caching`
		`-------`

		`.. automodule:: hubs.widgets.caching`
		`- :members:`
		`- :show-inheritance:`
		`+ :private-members:`

docs/conf.py

file modified

		`@@ -119,6 +119,9 @@`
		# If true, `todo` and `todoList` produce output, else they produce nothing.
		`todo_include_todos = True`

		`+ # Default flags for the autodoc extension: show public members (some private members need to be overriiden`
		`+ autodoc_default_flags = ['members', 'show-inheritance']`
jcline commented 6 years ago Neat, I didn't know about this
		`+`

		`# -- Options for HTML output ----------------------------------------------`

hubs/widgets/caching.py

file modified

+28 -9

		`@@ -34,16 +34,22 @@`
		`To call the function, instantiate the class and execute it. You may also`
		call the :py:meth:`.get_data` method.

		`- Args:`
		`+ Attributes:`
		`instance (hubs.models.Widget): The widget instance.`
		+ invalidate_on_config_change (bool): ``True`` if the cached result
		+ depends on the widget configuration, ``False`` otherwise. Defaults
		+ to ``True``. E.g: the function retrieves a lot of raw data from an
		`+ external service, and config-dependant filtering is done in the`
		`+ view calling the function.`
		`"""`

		`+ invalidate_on_config_change = True`
		`+`
		`def __init__(self, instance):`
		`self.instance = instance`

		`def execute(self):`
		`- """`
		`- The function to cache.`
		`+ """The function to cache.`

		`This is the main method, it must be implemented.`

		`@@ -53,20 +59,33 @@`
		`raise NotImplementedError`

		`def get_cache_key(self):`
		`- return "\|".join([`
		`- str(self.instance.idx),`
		`- self.__class__.__name__,`
		`- json.dumps(self.instance.config),`
		`- ]).encode('utf-8')`
		`+ key_elements = [str(self.instance.idx), self.__class__.__name__]`
		`+ if self.invalidate_on_config_change:`
		`+ key_elements.append(json.dumps(self.instance.config))`
		`+ return "\|".join(key_elements).encode('utf-8')`

		`def get_data(self):`
		`key = self.get_cache_key()`
		`log.debug("Accessing cache key %s", key)`
		`return cache.get_or_create(`
		`- key, self.execute)`
		`+ key, self.execute, should_cache_fn=self._should_cache)`

		`__call__ = get_data`

		`+ def _should_cache(self, value):`
jcline commented 6 years ago Is the intention here for this to be overridden by subclasses? If so, would it make more sense to make it part of the public interface by calling it `should_cache`?
		`+ """Indicates if the result should be cached.`
		`+`
		`+ This function which will receive the value returned by`
		+ :py:meth:`.execute`, and will then return ``True`` or ``False``,
		`+ indicating if the value should actually be cached or not. If it returns`
		+ ``False``, the value is still returned, but isn't cached. E.g.::
		`+`
		`+ return (value is not None)`
		`+`
		+ Defaults to ``True``.
		`+ """`
		`+ return True`
		`+`
		`def should_invalidate(self, message):`
		`"""`
		`Tell the cache invalidator if the received message should invalidate`

hubs/widgets/halp/functions.py

file modified

		`@@ -25,6 +25,7 @@`
		`them.`
		`"""`

		`+ invalidate_on_config_change = False`
		`TOPIC = "org.fedoraproject.prod.meetbot.meeting.item.help"`

		`def execute(self):`

abompard commented 6 years ago

Some functions retrieve a large amount of data to filter it later in the view. This is often the case with Datagrepper because the API does not allow complex filtering. As a result, the cache for this data does not need to be invalidated when a filter changes. This branch adds a variable so widgets can declare that.

There's also a commit allowing a cached function to declare what results should not be cached. For example if an HTTP request fails, it does not make sense to cache the empty result. This branch allows cached function to declare which values should not be cached (for example, None)

jcline commented on line 6 of docs/conf.py 6 years ago

Neat, I didn't know about this

jcline commented on line 50 of hubs/widgets/caching.py 6 years ago

Is the intention here for this to be overridden by subclasses? If so, would it make more sense to make it part of the public interface by calling it should_cache?

abompard commented 6 years ago

Thanks for the review. About making _should_cache public, this method is indeed intended to be overridden by subclasses, but not called from the outer code. That's why I think that one underscore prefix (a conventional equivalent to "protected") is more appropriate that the public interface.
But I'm very open to discussion :-)

rebased

6 years ago

rebased

6 years ago

rebased

6 years ago

rebased

6 years ago

Pull-Request has been merged by abompard

6 years ago