Merge pull request #14770 from edx/neem/waffle-utils

Waffle Switch with namespacing and request caching

cms/envs/test.py

@@ -196,6 +196,8 @@ simplefilter('ignore')
 CELERY_ALWAYS_EAGER = True
 CELERY_RESULT_BACKEND = 'djcelery.backends.cache:CacheBackend'
 
+CLEAR_REQUEST_CACHE_ON_TASK_COMPLETION = False
+
 ########################### Server Ports ###################################
 
 # These ports are carefully chosen so that if the browser needs to

common/djangoapps/request_cache/__init__.py

@@ -25,6 +25,7 @@ def clear_request_cache(**kwargs):  # pylint: disable=unused-argument
     Once a celery task completes, clear the request cache to
     prevent memory leaks.
     """
+    if getattr(settings, 'CLEAR_REQUEST_CACHE_ON_TASK_COMPLETION', True):
         middleware.RequestCache.clear_request_cache()
 
 

common/djangoapps/request_cache/tests.py

@@ -4,6 +4,7 @@ Tests for the request cache.
 from celery.task import task
 from django.conf import settings
 from django.test import TestCase
+from django.test.utils import override_settings
 from mock import Mock
 
 from request_cache import get_request_or_stub
@@ -31,6 +32,7 @@ class TestRequestCache(TestCase):
         cache = {"course_cache": "blah blah blah"}
         modulestore().request_cache.data.update(cache)
 
+    @override_settings(CLEAR_REQUEST_CACHE_ON_TASK_COMPLETION=True)
     def test_clear_cache_celery(self):
         """ Test that the request cache is cleared after a task is run. """
         self._dummy_task.apply(args=(self,)).get()

lms/djangoapps/course_api/blocks/tests/test_api.py

@@ -7,8 +7,7 @@ from django.test.client import RequestFactory
 from itertools import product
 
 from openedx.core.djangoapps.content.block_structure.api import clear_course_from_cache
-from openedx.core.djangoapps.content.block_structure.config import STORAGE_BACKING_FOR_CACHE
-from openedx.core.djangoapps.content.block_structure.tests.helpers import override_config_setting
+from openedx.core.djangoapps.content.block_structure.config import STORAGE_BACKING_FOR_CACHE, waffle
 
 from student.tests.factories import UserFactory
 from xmodule.modulestore import ModuleStoreEnum
@@ -142,12 +141,12 @@ class TestGetBlocksQueryCounts(SharedModuleStoreTestCase):
     )
     @ddt.unpack
     def test_query_counts_cached(self, store_type, with_storage_backing):
-        with override_config_setting(STORAGE_BACKING_FOR_CACHE, active=with_storage_backing):
+        with waffle().override(STORAGE_BACKING_FOR_CACHE, active=with_storage_backing):
             course = self._create_course(store_type)
             self._get_blocks(
                 course,
                 expected_mongo_queries=0,
-                expected_sql_queries=4 if with_storage_backing else 3,
+                expected_sql_queries=3 if with_storage_backing else 2,
             )
 
     @ddt.data(
@@ -159,7 +158,7 @@ class TestGetBlocksQueryCounts(SharedModuleStoreTestCase):
     @ddt.unpack
     def test_query_counts_uncached(self, store_type_tuple, with_storage_backing):
         store_type, expected_mongo_queries = store_type_tuple
-        with override_config_setting(STORAGE_BACKING_FOR_CACHE, active=with_storage_backing):
+        with waffle().override(STORAGE_BACKING_FOR_CACHE, active=with_storage_backing):
             course = self._create_course(store_type)
             clear_course_from_cache(course.id)
             self._get_blocks(

lms/envs/test.py

@@ -333,6 +333,8 @@ GIT_REPO_DIR = TEST_ROOT / "course_repos"
 CELERY_ALWAYS_EAGER = True
 CELERY_RESULT_BACKEND = 'djcelery.backends.cache:CacheBackend'
 
+CLEAR_REQUEST_CACHE_ON_TASK_COMPLETION = False
+
 ######################### MARKETING SITE ###############################
 
 MKTG_URL_LINK_MAP = {

openedx/core/djangoapps/content/block_structure/config/__init__.py

@@ -2,43 +2,27 @@
 This module contains various configuration settings via
 waffle switches for the Block Structure framework.
 """
-import logging
-
-from openedx.core.djangolib.waffle_utils import is_switch_enabled
-from request_cache.middleware import request_cached, RequestCache, func_call_cache_key
+from openedx.core.djangolib.waffle_utils import WaffleSwitchPlus
+from request_cache.middleware import request_cached
 
 from .models import BlockStructureConfiguration
 
 
-log = logging.getLogger(__name__)
-
+# Namespace
+WAFFLE_NAMESPACE = u'block_structure'
 
+# Switches
 INVALIDATE_CACHE_ON_PUBLISH = u'invalidate_cache_on_publish'
 STORAGE_BACKING_FOR_CACHE = u'storage_backing_for_cache'
 RAISE_ERROR_WHEN_NOT_FOUND = u'raise_error_when_not_found'
 PRUNE_OLD_VERSIONS = u'prune_old_versions'
 
 
-def is_enabled(setting_name):
-    """
-    Returns whether the given block_structure setting
-    is enabled.
-    """
-    bs_waffle_name = _bs_waffle_switch_name(setting_name)
-    return is_switch_enabled(bs_waffle_name)
-
-
-def enable_for_current_request(setting_name):
+def waffle():
     """
-    Enables the given block_structure setting for the
-    duration of the current request.
+    Returns the namespaced and cached Waffle class for BlockStructures.
     """
-    cache_key = func_call_cache_key(
-        is_switch_enabled.request_cached_contained_func,
-        _bs_waffle_switch_name(setting_name),
-    )
-    RequestCache.get_request_cache().data[cache_key] = True
-    log.warning(u'BlockStructure: Config %s is enabled for current request.', setting_name)
+    return WaffleSwitchPlus(namespace=WAFFLE_NAMESPACE, log_prefix=u'BlockStructure: ')
 
 
 @request_cached
@@ -55,11 +39,3 @@ def cache_timeout_in_seconds():
     Returns and caches the current setting for cache_timeout_in_seconds.
     """
     return BlockStructureConfiguration.current().cache_timeout_in_seconds
-
-
-def _bs_waffle_switch_name(setting_name):
-    """
-    Returns the name of the waffle switch for the
-    given block structure setting.
-    """
-    return u'block_structure.{}'.format(setting_name)

openedx/core/djangoapps/content/block_structure/management/commands/generate_course_blocks.py

@@ -7,7 +7,7 @@ from django.core.management.base import BaseCommand
 from xmodule.modulestore.django import modulestore
 
 import openedx.core.djangoapps.content.block_structure.api as api
-from openedx.core.djangoapps.content.block_structure.config import STORAGE_BACKING_FOR_CACHE, enable_for_current_request
+from openedx.core.djangoapps.content.block_structure.config import STORAGE_BACKING_FOR_CACHE, waffle
 import openedx.core.djangoapps.content.block_structure.tasks as tasks
 import openedx.core.djangoapps.content.block_structure.store as store
 from openedx.core.lib.command_utils import (
@@ -127,7 +127,7 @@ class Command(BaseCommand):
         Generates course blocks for the given course_keys per the given options.
         """
         if options.get('with_storage'):
-            enable_for_current_request(STORAGE_BACKING_FOR_CACHE)
+            waffle().override_for_request(STORAGE_BACKING_FOR_CACHE)
 
         for course_key in course_keys:
             try:

openedx/core/djangoapps/content/block_structure/manager.py

@@ -98,7 +98,7 @@ class BlockStructureManager(object):
             BlockStructureTransformers.verify_versions(block_structure)
 
         except (BlockStructureNotFound, TransformerDataIncompatible):
-            if config.is_enabled(config.RAISE_ERROR_WHEN_NOT_FOUND):
+            if config.waffle().is_enabled(config.RAISE_ERROR_WHEN_NOT_FOUND):
                 raise
             else:
                 block_structure = self._update_collected()

openedx/core/djangoapps/content/block_structure/models.py

@@ -226,7 +226,7 @@ class BlockStructureModel(TimeStampedModel):
         """
         Deletes previous file versions for data_usage_key.
         """
-        if not config.is_enabled(config.PRUNE_OLD_VERSIONS):
+        if not config.waffle().is_enabled(config.PRUNE_OLD_VERSIONS):
             return
 
         if num_to_keep is None:

openedx/core/djangoapps/content/block_structure/signals.py

@@ -23,7 +23,7 @@ def _update_block_structure_on_course_publish(sender, course_key, **kwargs):  #
     if isinstance(course_key, LibraryLocator):
         return
 
-    if config.is_enabled(config.INVALIDATE_CACHE_ON_PUBLISH):
+    if config.waffle().is_enabled(config.INVALIDATE_CACHE_ON_PUBLISH):
         clear_course_from_cache(course_key)
 
     update_course_in_cache_v2.apply_async(

openedx/core/djangoapps/content/block_structure/store.py

@@ -254,4 +254,4 @@ def _is_storage_backing_enabled():
     """
     Returns whether storage backing for Block Structures is enabled.
     """
-    return config.is_enabled(config.STORAGE_BACKING_FOR_CACHE)
+    return config.waffle().is_enabled(config.STORAGE_BACKING_FOR_CACHE)

openedx/core/djangoapps/content/block_structure/tasks.py

@@ -13,7 +13,7 @@ from opaque_keys.edx.keys import CourseKey
 
 from xmodule.modulestore.exceptions import ItemNotFoundError
 from openedx.core.djangoapps.content.block_structure import api
-from openedx.core.djangoapps.content.block_structure.config import STORAGE_BACKING_FOR_CACHE, enable_for_current_request
+from openedx.core.djangoapps.content.block_structure.config import STORAGE_BACKING_FOR_CACHE, waffle
 
 log = logging.getLogger('edx.celery.task')
 
@@ -59,7 +59,7 @@ def _update_course_in_cache(self, **kwargs):
     Updates the course blocks (mongo -> BlockStructure) for the specified course.
     """
     if kwargs.get('with_storage'):
-        enable_for_current_request(STORAGE_BACKING_FOR_CACHE)
+        waffle().override_for_request(STORAGE_BACKING_FOR_CACHE)
     _call_and_retry_if_needed(self, api.update_course_in_cache, **kwargs)
 
 
@@ -88,7 +88,7 @@ def _get_course_in_cache(self, **kwargs):
     Gets the course blocks for the specified course, updating the cache if needed.
     """
     if kwargs.get('with_storage'):
-        enable_for_current_request(STORAGE_BACKING_FOR_CACHE)
+        waffle().override_for_request(STORAGE_BACKING_FOR_CACHE)
     _call_and_retry_if_needed(self, api.get_course_in_cache, **kwargs)
 
 

openedx/core/djangoapps/content/block_structure/tests/helpers.py

@@ -7,11 +7,9 @@ from xmodule.modulestore.exceptions import ItemNotFoundError
 from uuid import uuid4
 
 from opaque_keys.edx.locator import CourseLocator, BlockUsageLocator
-from openedx.core.djangolib.testing.waffle_utils import override_switch
 
 from ..api import get_cache
 from ..block_structure import BlockStructureBlockData
-from ..config import _bs_waffle_switch_name
 from ..exceptions import BlockStructureNotFound
 from ..models import BlockStructureModel
 from ..store import BlockStructureStore
@@ -43,18 +41,6 @@ def is_course_in_block_structure_storage(course_key, store):
         return False
 
 
-class override_config_setting(override_switch):  # pylint:disable=invalid-name
-    """
-    Subclasses override_switch to use the block structure
-    name-spaced switch names.
-    """
-    def __init__(self, name, active):
-        super(override_config_setting, self).__init__(
-            _bs_waffle_switch_name(name),
-            active
-        )
-
-
 class MockXBlock(object):
     """
     A mock XBlock to be used in unit tests, thereby decoupling the

openedx/core/djangoapps/content/block_structure/tests/test_manager.py

@@ -6,7 +6,7 @@ from nose.plugins.attrib import attr
 from unittest import TestCase
 
 from ..block_structure import BlockStructureBlockData
-from ..config import RAISE_ERROR_WHEN_NOT_FOUND, STORAGE_BACKING_FOR_CACHE
+from ..config import RAISE_ERROR_WHEN_NOT_FOUND, STORAGE_BACKING_FOR_CACHE, waffle
 from ..exceptions import UsageKeyNotInBlockStructure, BlockStructureNotFound
 from ..manager import BlockStructureManager
 from ..transformers import BlockStructureTransformers
@@ -14,7 +14,6 @@ from .helpers import (
     MockModulestoreFactory, MockCache, MockTransformer,
     ChildrenMapTestMixin, UsageKeyFactoryMixin,
     mock_registered_transformers,
-    override_config_setting,
 )
 
 
@@ -173,14 +172,14 @@ class TestBlockStructureManager(UsageKeyFactoryMixin, ChildrenMapTestMixin, Test
         self.assertEquals(TestTransformer1.collect_call_count, 1)
 
     def test_get_collected_error_raised(self):
-        with override_config_setting(RAISE_ERROR_WHEN_NOT_FOUND, active=True):
+        with waffle().override(RAISE_ERROR_WHEN_NOT_FOUND, active=True):
             with mock_registered_transformers(self.registered_transformers):
                 with self.assertRaises(BlockStructureNotFound):
                     self.bs_manager.get_collected()
 
     @ddt.data(True, False)
     def test_update_collected_if_needed(self, with_storage_backing):
-        with override_config_setting(STORAGE_BACKING_FOR_CACHE, active=with_storage_backing):
+        with waffle().override(STORAGE_BACKING_FOR_CACHE, active=with_storage_backing):
             with mock_registered_transformers(self.registered_transformers):
                 self.assertEquals(TestTransformer1.collect_call_count, 0)
 

openedx/core/djangoapps/content/block_structure/tests/test_models.py

@@ -12,10 +12,9 @@ from uuid import uuid4
 
 from opaque_keys.edx.locator import CourseLocator, BlockUsageLocator
 
-from ..config import PRUNE_OLD_VERSIONS
+from ..config import PRUNE_OLD_VERSIONS, waffle
 from ..exceptions import BlockStructureNotFound
 from ..models import BlockStructureModel, _directory_name, _storage_error_handling
-from .helpers import override_config_setting
 
 
 @ddt.ddt
@@ -31,7 +30,7 @@ class BlockStructureModelTestCase(TestCase):
         self.params = self._create_bsm_params()
 
     def tearDown(self):
-        with override_config_setting(PRUNE_OLD_VERSIONS, active=True):
+        with waffle().override(PRUNE_OLD_VERSIONS, active=True):
             BlockStructureModel._prune_files(self.usage_key, num_to_keep=0)
         super(BlockStructureModelTestCase, self).tearDown()
 
@@ -105,18 +104,18 @@ class BlockStructureModelTestCase(TestCase):
         # old files not pruned
         self._assert_file_count_equal(2)
 
-    @override_config_setting(PRUNE_OLD_VERSIONS, active=True)
     @patch('openedx.core.djangoapps.content.block_structure.config.num_versions_to_keep', Mock(return_value=1))
     def test_prune_files(self):
+        with waffle().override(PRUNE_OLD_VERSIONS, active=True):
             self._verify_update_or_create_call('test data', expect_created=True)
             self._verify_update_or_create_call('updated data', expect_created=False)
             self._assert_file_count_equal(1)
 
-    @override_config_setting(PRUNE_OLD_VERSIONS, active=True)
     @patch('openedx.core.djangoapps.content.block_structure.config.num_versions_to_keep', Mock(return_value=1))
     @patch('openedx.core.djangoapps.content.block_structure.models.BlockStructureModel._delete_files')
     @patch('openedx.core.djangoapps.content.block_structure.models.log')
     def test_prune_exception(self, mock_log, mock_delete):
+        with waffle().override(PRUNE_OLD_VERSIONS, active=True):
             mock_delete.side_effect = Exception
             self._verify_update_or_create_call('test data', expect_created=True)
             self._verify_update_or_create_call('updated data', expect_created=False)
@@ -142,7 +141,7 @@ class BlockStructureModelTestCase(TestCase):
             if num_prior_edits:
                 self._assert_file_count_equal(num_prior_edits)
 
-            with override_config_setting(PRUNE_OLD_VERSIONS, active=True):
+            with waffle().override(PRUNE_OLD_VERSIONS, active=True):
                 self._verify_update_or_create_call('data')
                 self._assert_file_count_equal(min(prune_keep_count, num_prior_edits + 1))
 

openedx/core/djangoapps/content/block_structure/tests/test_signals.py

@@ -10,9 +10,9 @@ from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
 from xmodule.modulestore.tests.factories import CourseFactory
 
 from ..api import get_block_structure_manager
-from ..config import INVALIDATE_CACHE_ON_PUBLISH
+from ..config import INVALIDATE_CACHE_ON_PUBLISH, waffle
 from ..signals import _update_block_structure_on_course_publish
-from .helpers import is_course_in_block_structure_cache, override_config_setting
+from .helpers import is_course_in_block_structure_cache
 
 
 @ddt.ddt
@@ -54,7 +54,7 @@ class CourseBlocksSignalTest(ModuleStoreTestCase):
     def test_cache_invalidation(self, invalidate_cache_enabled, mock_bs_manager_clear):
         test_display_name = "Jedi 101"
 
-        with override_config_setting(INVALIDATE_CACHE_ON_PUBLISH, active=invalidate_cache_enabled):
+        with waffle().override(INVALIDATE_CACHE_ON_PUBLISH, active=invalidate_cache_enabled):
             self.course.display_name = test_display_name
             self.store.update_item(self.course, self.user.id)
 

openedx/core/djangoapps/content/block_structure/tests/test_store.py

@@ -6,11 +6,11 @@ from nose.plugins.attrib import attr
 
 from openedx.core.djangolib.testing.utils import CacheIsolationTestCase
 
-from ..config import STORAGE_BACKING_FOR_CACHE
+from ..config import STORAGE_BACKING_FOR_CACHE, waffle
 from ..config.models import BlockStructureConfiguration
 from ..exceptions import BlockStructureNotFound
 from ..store import BlockStructureStore
-from .helpers import ChildrenMapTestMixin, UsageKeyFactoryMixin, MockCache, MockTransformer, override_config_setting
+from .helpers import ChildrenMapTestMixin, UsageKeyFactoryMixin, MockCache, MockTransformer
 
 
 @attr(shard=2)
@@ -47,13 +47,13 @@ class TestBlockStructureStore(UsageKeyFactoryMixin, ChildrenMapTestMixin, CacheI
 
     @ddt.data(True, False)
     def test_get_none(self, with_storage_backing):
-        with override_config_setting(STORAGE_BACKING_FOR_CACHE, active=with_storage_backing):
+        with waffle().override(STORAGE_BACKING_FOR_CACHE, active=with_storage_backing):
             with self.assertRaises(BlockStructureNotFound):
                 self.store.get(self.block_structure.root_block_usage_key)
 
     @ddt.data(True, False)
     def test_add_and_get(self, with_storage_backing):
-        with override_config_setting(STORAGE_BACKING_FOR_CACHE, active=with_storage_backing):
+        with waffle().override(STORAGE_BACKING_FOR_CACHE, active=with_storage_backing):
             self.store.add(self.block_structure)
             stored_value = self.store.get(self.block_structure.root_block_usage_key)
             self.assertIsNotNone(stored_value)
@@ -61,7 +61,7 @@ class TestBlockStructureStore(UsageKeyFactoryMixin, ChildrenMapTestMixin, CacheI
 
     @ddt.data(True, False)
     def test_delete(self, with_storage_backing):
-        with override_config_setting(STORAGE_BACKING_FOR_CACHE, active=with_storage_backing):
+        with waffle().override(STORAGE_BACKING_FOR_CACHE, active=with_storage_backing):
             self.store.add(self.block_structure)
             self.store.delete(self.block_structure.root_block_usage_key)
             with self.assertRaises(BlockStructureNotFound):
@@ -74,7 +74,7 @@ class TestBlockStructureStore(UsageKeyFactoryMixin, ChildrenMapTestMixin, CacheI
             self.store.get(self.block_structure.root_block_usage_key)
 
     def test_uncached_with_storage(self):
-        with override_config_setting(STORAGE_BACKING_FOR_CACHE, active=True):
+        with waffle().override(STORAGE_BACKING_FOR_CACHE, active=True):
             self.store.add(self.block_structure)
             self.mock_cache.map.clear()
             stored_value = self.store.get(self.block_structure.root_block_usage_key)

openedx/core/djangolib/testing/waffle_utils.py

-"""
-Test utilities when using waffle.
-"""
-from waffle.testutils import override_switch as waffle_override_switch
-from request_cache.middleware import RequestCache, func_call_cache_key
-from ..waffle_utils import is_switch_enabled
-
-
-class override_switch(waffle_override_switch):  # pylint:disable=invalid-name
-    """
-    Subclasses waffle's override_switch in order clear the cache
-    used on the is_switch_enabled function.
-    """
-    def _clear_cache(self):
-        """
-        Clears the requestcached values on the is_switch_enabled function.
-        """
-        cache_key = func_call_cache_key(is_switch_enabled.request_cached_contained_func, self.name)
-        RequestCache.get_request_cache().data.pop(cache_key, None)
-
-    def __enter__(self):
-        self._clear_cache()
-        super(override_switch, self).__enter__()
-
-    def __exit__(self, *args, **kwargs):
-        self._clear_cache()
-        super(override_switch, self).__exit__(*args, **kwargs)

openedx/core/djangolib/waffle_utils.py

 """
 Utilities for waffle usage.
 """
-from request_cache.middleware import request_cached
+from abc import ABCMeta
+from contextlib import contextmanager
+import logging
+
+from waffle.models import Switch
+from waffle.utils import get_setting as waffle_setting
 from waffle import switch_is_active
 
+from request_cache import get_cache as get_request_cache
+
+
+log = logging.getLogger(__name__)
+
+
+class WafflePlus(object):
+    """
+    Waffle helper class that provides native support for
+    namespacing waffle settings and caching within a request.
+    """
+    __metaclass__ = ABCMeta
+
+    def __init__(self, namespace, log_prefix=None):
+        self.namespace = namespace
+        self.log_prefix = log_prefix
+
+    def _namespaced_setting_name(self, setting_name):
+        """
+        Returns the namespaced name of the waffle switch/flag.
+        """
+        assert self.namespace is not None
+        return u'{}.{}'.format(self.namespace, setting_name)
+
+    @staticmethod
+    def _get_request_cache():
+        """
+        Returns the request cache used by WafflePlus classes.
+        """
+        return get_request_cache('WafflePlus')
+
 
-@request_cached
-def is_switch_enabled(waffle_name):
+class WaffleSwitchPlus(WafflePlus):
+    """
+    Waffle Switch helper class that provides native support for
+    namespacing waffle switches and caching within a request.
+    """
+    def is_enabled(self, switch_name):
         """
         Returns and caches whether the given waffle switch is enabled.
-    See testing.waffle_utils.override_config_setting for a
-    helper to override and clear the cache during tests.
         """
-    return switch_is_active(waffle_name)
+        namespaced_switch_name = self._namespaced_setting_name(switch_name)
+        value = self._cached_switches.get(namespaced_switch_name)
+        if value is None:
+            value = switch_is_active(namespaced_switch_name)
+            self._cached_switches[namespaced_switch_name] = value
+        return value
+
+    @contextmanager
+    def override(self, switch_name, active=True):
+        """
+        Overrides the active value for the given switch for the duration of this
+        contextmanager.
+        Note: The value is overridden in the request cache, not in the model.
+        """
+        previous_active = self.is_enabled(switch_name)
+        try:
+            self.override_for_request(switch_name, active)
+            yield
+        finally:
+            self.override_for_request(switch_name, previous_active)
+
+    def override_for_request(self, switch_name, active=True):
+        """
+        Overrides the active value for the given switch for the remainder of
+        this request (as this is not a context manager).
+        Note: The value is overridden in the request cache, not in the model.
+        """
+        namespaced_switch_name = self._namespaced_setting_name(switch_name)
+        self._cached_switches[namespaced_switch_name] = active
+        log.info(u"%sSwitch '%s' set to %s for request.", self.log_prefix, namespaced_switch_name, active)
+
+    @property
+    def _cached_switches(self):
+        """
+        Returns cached active values of all switches in this namespace.
+        """
+        return self._all_cached_switches.setdefault(self.namespace, {})
+
+    @property
+    def _all_cached_switches(self):
+        """
+        Returns dictionary of all switches in the request cache,
+        keyed by namespace.
+        """
+        return self._get_request_cache().setdefault('switches', {})