Merge pull request #14504 from edx/beryl/cbs_transformer_versions

Block Transformers: distinguish between READ and WRITE versions

lms/djangoapps/course_api/blocks/transformers/block_counts.py

@@ -8,7 +8,8 @@ class BlockCountsTransformer(BlockStructureTransformer):
     """
     Keep a count of descendant blocks of the requested types
     """
-    VERSION = 1
+    WRITE_VERSION = 1
+    READ_VERSION = 1
     BLOCK_COUNTS = 'block_counts'
 
     def __init__(self, block_types_to_count):

lms/djangoapps/course_api/blocks/transformers/block_depth.py

@@ -9,7 +9,8 @@ class BlockDepthTransformer(BlockStructureTransformer):
     Keep track of the depth of each block within the block structure.  In case
     of multiple paths to a given node (in a DAG), use the shallowest depth.
     """
-    VERSION = 1
+    WRITE_VERSION = 1
+    READ_VERSION = 1
     BLOCK_DEPTH = 'block_depth'
 
     def __init__(self, requested_depth=None):

lms/djangoapps/course_api/blocks/transformers/blocks_api.py

@@ -22,7 +22,8 @@ class BlocksAPITransformer(BlockStructureTransformer):
     Note: BlockDepthTransformer must be executed before BlockNavigationTransformer.
     """
 
-    VERSION = 1
+    WRITE_VERSION = 1
+    READ_VERSION = 1
     STUDENT_VIEW_DATA = 'student_view_data'
     STUDENT_VIEW_MULTI_DEVICE = 'student_view_multi_device'
 

lms/djangoapps/course_api/blocks/transformers/milestones.py

@@ -13,7 +13,8 @@ class MilestonesTransformer(FilteringTransformerMixin, BlockStructureTransformer
     Excludes all special exams (timed, proctored, practice proctored) from the student view.
     Excludes all blocks with unfulfilled milestones from the student view.
     """
-    VERSION = 1
+    WRITE_VERSION = 1
+    READ_VERSION = 1
 
     @classmethod
     def name(cls):

lms/djangoapps/course_api/blocks/transformers/navigation.py

@@ -20,7 +20,8 @@ class BlockNavigationTransformer(BlockStructureTransformer):
     Prerequisites: BlockDepthTransformer must be run before this in the
     transform phase.
     """
-    VERSION = 1
+    WRITE_VERSION = 1
+    READ_VERSION = 1
     BLOCK_NAVIGATION = 'block_nav'
     BLOCK_NAVIGATION_FOR_CHILDREN = 'children_block_nav'
 

lms/djangoapps/course_api/blocks/transformers/student_view.py

@@ -8,7 +8,8 @@ class StudentViewTransformer(BlockStructureTransformer):
     """
     Only show information that is appropriate for a learner
     """
-    VERSION = 1
+    WRITE_VERSION = 1
+    READ_VERSION = 1
     STUDENT_VIEW_DATA = 'student_view_data'
     STUDENT_VIEW_MULTI_DEVICE = 'student_view_multi_device'
 

lms/djangoapps/course_blocks/transformers/hidden_content.py

@@ -25,7 +25,8 @@ class HiddenContentTransformer(FilteringTransformerMixin, BlockStructureTransfor
 
     Staff users are exempted from hidden content rules.
     """
-    VERSION = 2
+    WRITE_VERSION = 2
+    READ_VERSION = 2
     MERGED_DUE_DATE = 'merged_due_date'
     MERGED_HIDE_AFTER_DUE = 'merged_hide_after_due'
 

lms/djangoapps/course_blocks/transformers/library_content.py

@@ -18,7 +18,8 @@ class ContentLibraryTransformer(FilteringTransformerMixin, BlockStructureTransfo
 
     Staff users are *not* exempted from library content pathways.
     """
-    VERSION = 1
+    WRITE_VERSION = 1
+    READ_VERSION = 1
 
     @classmethod
     def name(cls):

lms/djangoapps/course_blocks/transformers/split_test.py

@@ -19,7 +19,8 @@ class SplitTestTransformer(FilteringTransformerMixin, BlockStructureTransformer)
     to actually enforce the access using the 'user_partitions' and
     'group_access' fields.
     """
-    VERSION = 1
+    WRITE_VERSION = 1
+    READ_VERSION = 1
 
     @classmethod
     def name(cls):

lms/djangoapps/course_blocks/transformers/start_date.py

@@ -24,7 +24,8 @@ class StartDateTransformer(FilteringTransformerMixin, BlockStructureTransformer)
 
     Staff users are exempted from visibility rules.
     """
-    VERSION = 1
+    WRITE_VERSION = 1
+    READ_VERSION = 1
     MERGED_START_DATE = 'merged_start_date'
 
     @classmethod

lms/djangoapps/course_blocks/transformers/user_partitions.py

@@ -16,7 +16,8 @@ class UserPartitionTransformer(FilteringTransformerMixin, BlockStructureTransfor
 
     Staff users are *not* exempted from user partition pathways.
     """
-    VERSION = 1
+    WRITE_VERSION = 1
+    READ_VERSION = 1
 
     @classmethod
     def name(cls):

lms/djangoapps/course_blocks/transformers/visibility.py

@@ -18,7 +18,8 @@ class VisibilityTransformer(FilteringTransformerMixin, BlockStructureTransformer
 
     Staff users are exempted from visibility rules.
     """
-    VERSION = 1
+    WRITE_VERSION = 1
+    READ_VERSION = 1
 
     MERGED_VISIBLE_TO_STAFF_ONLY = 'merged_visible_to_staff_only'
 

lms/djangoapps/grades/tests/test_tasks.py

@@ -13,6 +13,7 @@ from mock import patch, MagicMock
 import pytz
 from util.date_utils import to_timestamp
 
+from openedx.core.lib.block_structure.exceptions import BlockStructureNotFound
 from student.models import anonymous_id_for_user
 from student.tests.factories import UserFactory
 from track.event_transaction_utils import (
@@ -130,7 +131,7 @@ class RecalculateSubsectionGradeTest(ModuleStoreTestCase):
         self.assertTrue(PersistentGradesEnabledFlag.feature_enabled(self.course.id))
         with patch(
             'openedx.core.lib.block_structure.factory.BlockStructureFactory.create_from_cache',
-            return_value=None,
+            side_effect=BlockStructureNotFound,
         ) as mock_block_structure_create:
             self._apply_recalculate_subsection_grade()
             self.assertEquals(mock_block_structure_create.call_count, 1)

lms/djangoapps/grades/transformer.py

@@ -2,17 +2,13 @@
 Grades Transformer
 """
 from base64 import b64encode
-from django.test.client import RequestFactory
 from functools import reduce as functools_reduce
 from hashlib import sha1
 from logging import getLogger
 import json
 
-from courseware.model_data import FieldDataCache
-import courseware.module_render
 from lms.djangoapps.course_blocks.transformers.utils import collect_unioned_set_field, get_field_on_block
 from openedx.core.lib.block_structure.transformer import BlockStructureTransformer
-from openedx.core.djangoapps.util.user_utils import SystemUser
 
 
 log = getLogger(__name__)
@@ -39,7 +35,8 @@ class GradesTransformer(BlockStructureTransformer):
 
         max_score: (numeric)
     """
-    VERSION = 4
+    WRITE_VERSION = 4
+    READ_VERSION = 4
     FIELDS_TO_COLLECT = [u'due', u'format', u'graded', u'has_score', u'weight', u'course_version', u'subtree_edited_on']
 
     EXPLICIT_GRADED_FIELD_NAME = 'explicit_graded'

openedx/core/lib/block_structure/block_structure.py

@@ -730,9 +730,9 @@ class BlockStructureBlockData(BlockStructure):
         Adds the given transformer to the block structure by recording
         its current version number.
         """
-        if transformer.VERSION == 0:
-            raise TransformerException('VERSION attribute is not set on transformer {0}.', transformer.name())
-        self.set_transformer_data(transformer, TRANSFORMER_VERSION_KEY, transformer.VERSION)
+        if transformer.READ_VERSION == 0 or transformer.WRITE_VERSION == 0:
+            raise TransformerException('Version attributes are not set on transformer {0}.', transformer.name())
+        self.set_transformer_data(transformer, TRANSFORMER_VERSION_KEY, transformer.WRITE_VERSION)
 
     def _get_or_create_block(self, usage_key):
         """

openedx/core/lib/block_structure/exceptions.py

@@ -3,15 +3,37 @@ Application-specific exceptions raised by the block structure framework.
 """
 
 
-class TransformerException(Exception):
+class BlockStructureException(Exception):
+    """
+    Base class for all Block Structure framework exceptions.
+    """
+    pass
+
+
+class TransformerException(BlockStructureException):
     """
     Exception class for Transformer related errors.
     """
     pass
 
 
-class UsageKeyNotInBlockStructure(Exception):
+class UsageKeyNotInBlockStructure(BlockStructureException):
     """
     Exception for when a usage key is not found within a block structure.
     """
     pass
+
+
+class TransformerDataIncompatible(BlockStructureException):
+    """
+    Exception for when the version of a Transformer's data is not
+    compatible with the current version of the Transformer.
+    """
+    pass
+
+
+class BlockStructureNotFound(BlockStructureException):
+    """
+    Exception for when a Block Structure is not found.
+    """
+    pass

openedx/core/lib/block_structure/factory.py

@@ -2,6 +2,7 @@
 Module for factory class for BlockStructure objects.
 """
 from .block_structure import BlockStructureModulestoreData, BlockStructureBlockData
+from .exceptions import BlockStructureNotFound
 
 
 class BlockStructureFactory(object):
@@ -77,11 +78,16 @@ class BlockStructureFactory(object):
 
         Returns:
             BlockStructure - The deserialized block structure starting
-            at root_block_usage_key, if found in the cache.
+                at root_block_usage_key, if found in the cache.
 
-            NoneType - If the root_block_usage_key is not found in the cache.
+        Raises:
+            BlockStructureNotFound - If the root_block_usage_key is not found
+                in the cache.
         """
-        return block_structure_cache.get(root_block_usage_key)
+        block_structure = block_structure_cache.get(root_block_usage_key)
+        if block_structure is None:
+            raise BlockStructureNotFound('Block structure for {} not found in the cache.'.format(root_block_usage_key))
+        return block_structure
 
     @classmethod
     def create_new(cls, root_block_usage_key, block_relations, transformer_data, block_data_map):

openedx/core/lib/block_structure/manager.py

@@ -6,7 +6,7 @@ from contextlib import contextmanager
 
 from .cache import BlockStructureCache
 from .factory import BlockStructureFactory
-from .exceptions import UsageKeyNotInBlockStructure
+from .exceptions import UsageKeyNotInBlockStructure, TransformerDataIncompatible, BlockStructureNotFound
 from .transformers import BlockStructureTransformers
 
 
@@ -89,13 +89,16 @@ class BlockStructureManager(object):
                 starting at root_block_usage_key, with collected data
                 from each registered transformer.
         """
-        block_structure = BlockStructureFactory.create_from_cache(
-            self.root_block_usage_key,
-            self.block_structure_cache
-        )
-        cache_miss = block_structure is None
-        if cache_miss or BlockStructureTransformers.is_collected_outdated(block_structure):
+        try:
+            block_structure = BlockStructureFactory.create_from_cache(
+                self.root_block_usage_key,
+                self.block_structure_cache
+            )
+            BlockStructureTransformers.verify_versions(block_structure)
+
+        except (BlockStructureNotFound, TransformerDataIncompatible):
             block_structure = self.update_collected()
+
         return block_structure
 
     def update_collected(self):

openedx/core/lib/block_structure/tests/helpers.py

@@ -136,7 +136,8 @@ class MockTransformer(BlockStructureTransformer):
     """
     A mock BlockStructureTransformer class.
     """
-    VERSION = 1
+    WRITE_VERSION = 1
+    READ_VERSION = 1
 
     @classmethod
     def name(cls):
@@ -151,7 +152,8 @@ class MockFilteringTransformer(FilteringTransformerMixin, BlockStructureTransfor
     """
     A mock FilteringTransformerMixin class.
     """
-    VERSION = 1
+    WRITE_VERSION = 1
+    READ_VERSION = 1
 
     @classmethod
     def name(cls):

openedx/core/lib/block_structure/tests/test_block_structure.py

@@ -56,11 +56,12 @@ class TestBlockStructureData(TestCase, ChildrenMapTestMixin):
             """
             Test transformer with default version number (0).
             """
-            VERSION = 0
+            WRITE_VERSION = 0
+            READ_VERSION = 0
 
         block_structure = BlockStructureModulestoreData(root_block_usage_key=0)
 
-        with self.assertRaisesRegexp(TransformerException, "VERSION attribute is not set"):
+        with self.assertRaisesRegexp(TransformerException, "Version attributes are not set"):
             block_structure._add_transformer(TestNonVersionedTransformer())
 
     def test_transformer_data(self):
@@ -103,7 +104,7 @@ class TestBlockStructureData(TestCase, ChildrenMapTestMixin):
         for t_info in transformers_info:
             self.assertEquals(
                 block_structure._get_transformer_data_version(t_info.transformer),
-                MockTransformer.VERSION
+                MockTransformer.WRITE_VERSION
             )
             for key, val in t_info.structure_wide_data:
                 self.assertEquals(

openedx/core/lib/block_structure/tests/test_factory.py

@@ -6,6 +6,7 @@ from unittest import TestCase
 from xmodule.modulestore.exceptions import ItemNotFoundError
 
 from ..cache import BlockStructureCache
+from ..exceptions import BlockStructureNotFound
 from ..factory import BlockStructureFactory
 from .helpers import (
     MockCache, MockModulestoreFactory, ChildrenMapTestMixin
@@ -43,17 +44,15 @@ class TestBlockStructureFactory(TestCase, ChildrenMapTestMixin):
             block_structure.root_block_usage_key,
             cache,
         )
-        self.assertIsNotNone(from_cache_block_structure)
         self.assert_block_structure(from_cache_block_structure, self.children_map)
 
     def test_from_cache_none(self):
         cache = BlockStructureCache(MockCache())
-        self.assertIsNone(
+        with self.assertRaises(BlockStructureNotFound):
             BlockStructureFactory.create_from_cache(
                 root_block_usage_key=0,
                 block_structure_cache=cache,
             )
-        )
 
     def test_new(self):
         block_structure = BlockStructureFactory.create_from_modulestore(

openedx/core/lib/block_structure/tests/test_manager.py

@@ -167,15 +167,24 @@ class TestBlockStructureManager(TestCase, ChildrenMapTestMixin):
         self.collect_and_verify(expect_modulestore_called=False, expect_cache_updated=False)
         self.assertEquals(TestTransformer1.collect_call_count, 1)
 
-    def test_get_collected_outdated_data(self):
+    def test_get_collected_transformer_version(self):
         self.collect_and_verify(expect_modulestore_called=True, expect_cache_updated=True)
-        TestTransformer1.VERSION += 1  # transformer code requires new schema version
+
+        # transformer code writes new schema version; data not re-collected
+        TestTransformer1.WRITE_VERSION += 1
+        self.collect_and_verify(expect_modulestore_called=False, expect_cache_updated=False)
+
+        # transformer code requires new schema version; data re-collected
+        TestTransformer1.READ_VERSION += 1
         self.collect_and_verify(expect_modulestore_called=True, expect_cache_updated=True)
-        TestTransformer1.VERSION -= 1  # old transformer code works with new schema version
+
+        # old transformer code can read new schema version; data not re-collected
+        TestTransformer1.READ_VERSION -= 1
         self.collect_and_verify(expect_modulestore_called=False, expect_cache_updated=False)
+
         self.assertEquals(TestTransformer1.collect_call_count, 2)
 
-    def test_get_collected_version_update(self):
+    def test_get_collected_structure_version(self):
         self.collect_and_verify(expect_modulestore_called=True, expect_cache_updated=True)
         BlockStructureBlockData.VERSION += 1
         self.collect_and_verify(expect_modulestore_called=True, expect_cache_updated=True)

openedx/core/lib/block_structure/tests/test_transformers.py

@@ -6,7 +6,7 @@ from nose.plugins.attrib import attr
 from unittest import TestCase
 
 from ..block_structure import BlockStructureModulestoreData
-from ..exceptions import TransformerException
+from ..exceptions import TransformerException, TransformerDataIncompatible
 from ..transformers import BlockStructureTransformers
 from .helpers import (
     ChildrenMapTestMixin, MockTransformer, MockFilteringTransformer, mock_registered_transformers
@@ -71,13 +71,14 @@ class TestBlockStructureTransformers(ChildrenMapTestMixin, TestCase):
             self.transformers.transform(block_structure=MagicMock())
             self.assertTrue(mock_transform_call.called)
 
-    def test_is_collected_outdated(self):
+    def test_verify_versions(self):
         block_structure = self.create_block_structure(
             self.SIMPLE_CHILDREN_MAP,
             BlockStructureModulestoreData
         )
 
         with mock_registered_transformers(self.registered_transformers):
-            self.assertTrue(self.transformers.is_collected_outdated(block_structure))
+            with self.assertRaises(TransformerDataIncompatible):
+                self.transformers.verify_versions(block_structure)
             self.transformers.collect(block_structure)
-            self.assertFalse(self.transformers.is_collected_outdated(block_structure))
+            self.assertTrue(self.transformers.verify_versions(block_structure))

openedx/core/lib/block_structure/transformer.py

@@ -10,23 +10,50 @@ class BlockStructureTransformer(object):
     Abstract base class for all block structure transformers.
     """
 
-    # All Transformers are expected to maintain a VERSION class
-    # attribute.  While the value for the base class is set to 0,
-    # the value for each concrete transformer should be 1 or higher.
+    # All Transformers are expected to maintain version-related class
+    # attributes.  While the values for the base class is set to 0,
+    # the values for each concrete transformer should be 1 or higher.
     #
-    # A transformer's version attribute is used by the block_structure
+    # A transformer's version attributes are used by the block_structure
     # framework in order to determine whether any collected data for a
-    # transformer is outdated.  When a transformer's data is collected
-    # and cached, it's version number at the time of collection is
-    # stored along with the data.  That version number is then checked
-    # at the time of accessing the collected data (during the transform
-    # phase).
+    # transformer is outdated because of a data schema change by the
+    # transformer.
     #
-    # The version number of a Transformer should be incremented each
-    # time the implementation of its collect method is updated such that
-    # its collected data is changed.
+    # The WRITE_VERSION number is stored along with the transformer's
+    # data when it is collected and cached (during the collect phase).
+    # The READ_VERSION number is then verified to be less than or equal
+    # to the version associated with the collected data when the
+    # collected data is accessed (during the transform phase).
     #
-    VERSION = 0
+    # We distinguish between WRITE_VERSION and READ_VERSION numbers in
+    # order to:
+    # 1. support blue-green deployments where new and previous versions
+    #    of the code base are simultaneously executing on different
+    #    workers for a period of time.
+    #
+    #    A 2-phase deployment is used to stagger read and write changes.
+    #
+    # 2. scale for large deployments where it is costly to recompute
+    #    block structures for all courses when a transformer's collected
+    #    data schema changes.
+    #
+    #    A background management command is run to prime the new data.
+    #
+    # See the following document for further information:
+    # https://openedx.atlassian.net/wiki/display/MA/Block+Structure+Cache+Invalidation+Proposal
+    #
+    # The WRITE_VERSION number of a Transformer should be incremented
+    # when it's collect implementation is additively changed. Backward
+    # compatibility should be maintained with previous READ_VERSIONs
+    # until all readers are updated.
+    #
+    # The READ_VERSION number of a Transformer should be incremented
+    # when its transform implementation is updated to make use of the
+    # newly collected data - and released only after all collected
+    # block structures are updated with the new WRITE_VERSION.
+    #
+    WRITE_VERSION = 0
+    READ_VERSION = 0
 
     @classmethod
     def name(cls):

openedx/core/lib/block_structure/transformers.py

@@ -4,7 +4,7 @@ Module for a collection of BlockStructureTransformers.
 import functools
 from logging import getLogger
 
-from .exceptions import TransformerException
+from .exceptions import TransformerException, TransformerDataIncompatible
 from .transformer import FilteringTransformerMixin
 from .transformer_registry import TransformerRegistry
 
@@ -83,23 +83,27 @@ class BlockStructureTransformers(object):
         block_structure._collect_requested_xblock_fields()  # pylint: disable=protected-access
 
     @classmethod
-    def is_collected_outdated(cls, block_structure):
+    def verify_versions(cls, block_structure):
         """
-        Returns whether the collected data in the block structure is outdated.
+        Returns whether the collected data in the block structure is
+        incompatible with the current version of the registered Transformers.
+
+        Raises:
+            TransformerDataIncompatible with information about all outdated
+            Transformers.
         """
         outdated_transformers = []
         for transformer in TransformerRegistry.get_registered_transformers():
             version_in_block_structure = block_structure._get_transformer_data_version(transformer)  # pylint: disable=protected-access
-            if transformer.VERSION > version_in_block_structure:
+            if transformer.READ_VERSION > version_in_block_structure:
                 outdated_transformers.append(transformer)
 
         if outdated_transformers:
-            logger.info(
+            raise TransformerDataIncompatible(
                 "Collected Block Structure data for the following transformers is outdated: '%s'.",
-                [(transformer.name(), transformer.VERSION) for transformer in outdated_transformers],
+                [(transformer.name(), transformer.READ_VERSION) for transformer in outdated_transformers],
             )
-
-        return bool(outdated_transformers)
+        return True
 
     def transform(self, block_structure):
         """