From 274e6de22bcbbf8cd4d6eeb346622037761d6e5b Mon Sep 17 00:00:00 2001 From: utkjad Date: Tue, 14 Jul 2015 20:19:04 +0000 Subject: [PATCH] PLAT-734 Fix slow transaction due to multiple calls to unpickling --- .../contentstore/views/tests/test_assets.py | 27 ++++++------------- .../xmodule/xmodule/assetstore/assetmgr.py | 20 +++++++------- 2 files changed, 17 insertions(+), 30 deletions(-) diff --git a/cms/djangoapps/contentstore/views/tests/test_assets.py b/cms/djangoapps/contentstore/views/tests/test_assets.py index 3fc5646ac7..78ce5b4050 100644 --- a/cms/djangoapps/contentstore/views/tests/test_assets.py +++ b/cms/djangoapps/contentstore/views/tests/test_assets.py @@ -6,13 +6,12 @@ from io import BytesIO from pytz import UTC from PIL import Image import json - +from mock import patch from django.conf import settings from contentstore.tests.utils import CourseTestCase from contentstore.views import assets from contentstore.utils import reverse_course_url -from xmodule.assetstore.assetmgr import AssetMetadataFoundTemporary from xmodule.assetstore import AssetMetadata from xmodule.contentstore.content import StaticContent from xmodule.contentstore.django import contentstore @@ -331,23 +330,13 @@ class DownloadTestCase(AssetsTestCase): resp = self.client.get(url, HTTP_ACCEPT='text/html') self.assertEquals(resp.status_code, 404) - def test_metadata_found_in_modulestore(self): - # Insert asset metadata into the modulestore (with no accompanying asset). - asset_key = self.course.id.make_asset_key(AssetMetadata.GENERAL_ASSET_TYPE, 'pic1.jpg') - asset_md = AssetMetadata(asset_key, { - 'internal_name': 'EKMND332DDBK', - 'basename': 'pix/archive', - 'locked': False, - 'curr_version': '14', - 'prev_version': '13' - }) - modulestore().save_asset_metadata(asset_md, 15) - # Get the asset metadata and have it be found in the modulestore. - # Currently, no asset metadata should be found in the modulestore. The code is not yet storing it there. - # If asset metadata *is* found there, an exception is raised. This test ensures the exception is indeed raised. - # THIS IS TEMPORARY. Soon, asset metadata *will* be stored in the modulestore. - with self.assertRaises((AssetMetadataFoundTemporary, NameError)): - self.client.get(unicode(asset_key), HTTP_ACCEPT='text/html') + @patch('xmodule.modulestore.mixed.MixedModuleStore.find_asset_metadata') + def test_pickling_calls(self, patched_find_asset_metadata): + """ Tests if assets are not calling find_asset_metadata + """ + patched_find_asset_metadata.return_value = None + self.client.get(self.uploaded_url, HTTP_ACCEPT='text/html') + self.assertFalse(patched_find_asset_metadata.called) class AssetToJsonTestCase(AssetsTestCase): diff --git a/common/lib/xmodule/xmodule/assetstore/assetmgr.py b/common/lib/xmodule/xmodule/assetstore/assetmgr.py index 601e827c77..fef81b05f1 100644 --- a/common/lib/xmodule/xmodule/assetstore/assetmgr.py +++ b/common/lib/xmodule/xmodule/assetstore/assetmgr.py @@ -9,11 +9,12 @@ Handles: Phase 1: Checks to see if an asset's metadata can be found in the course's modulestore. If not found, fails over to access the asset from the contentstore. At first, the asset metadata will never be found, since saving isn't implemented yet. +Note: Hotfix (PLAT-734) No asset calls find_asset_metadata, and directly accesses from contentstore. + """ from contracts import contract, new_contract from opaque_keys.edx.keys import AssetKey -from xmodule.modulestore.django import modulestore from xmodule.contentstore.django import contentstore @@ -49,14 +50,11 @@ class AssetManager(object): @contract(asset_key='AssetKey', throw_on_not_found='bool', as_stream='bool') def find(asset_key, throw_on_not_found=True, as_stream=False): """ - Finds a course asset either in the assetstore -or- in the deprecated contentstore. + Finds course asset in the deprecated contentstore. + This method was previously searching for the course asset in the assetstore first, then in the deprecated + contentstore. However, the asset was never found in the assetstore since an asset's metadata is + not yet stored there.(removed calls to modulestore().find_asset_metadata(asset_key)) + The assetstore search was removed due to performance issues caused by each call unpickling the pickled and + compressed course structure from the structure cache. """ - content_md = modulestore().find_asset_metadata(asset_key) - - # If found, raise an exception. - if content_md: - # For now, no asset metadata should be found in the modulestore. - raise AssetMetadataFoundTemporary() - else: - # If not found, load the asset via the contentstore. - return contentstore().find(asset_key, throw_on_not_found, as_stream) + return contentstore().find(asset_key, throw_on_not_found, as_stream)