perf: provide option to query asset using storage default url or through the LMS #100
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Danyal-Faheem
changed the title
…gh LMS This was provided as proxying assets through the LMS can be slower than directly accessing from external storage The default value will be to proxy assets through the LMS as it is more reliable
Danyal-Faheem
force-pushed
the
danyal/storage-url-option
branch
from
2060ed7 to
e6f8fda
Compare
ziafazal
reviewed
Mar 10, 2025
| @@ -420,14 +383,20 @@ def extract_package(self, package_file): | |||
| def index_page_url(self): | |||
| if not self.package_meta or not self.index_page_path: | |||
| return "" | |||
|
|
|||
| if not self.xblock_settings.get("DEFAULT_STORAGE_URL", False): | |||
There was a problem hiding this comment.
I think we should use S3_QUERY_AUTH setting to determinse whetere we should serve static assets vis lms or not. because this setting is used for signing urls in case of private bucket.
Suggested change
| if not self.xblock_settings.get("DEFAULT_STORAGE_URL", False): | |
| if self.xblock_settings.get("S3_QUERY_AUTH",True) |
| @@ -55,43 +55,6 @@ def _(text): | |||
| @XBlock.wants("settings") | |||
| @XBlock.wants("user") | |||
| class ScormXBlock(XBlock, CompletableXBlockMixin): | |||
| """ | |||
There was a problem hiding this comment.
why we need to remove this docstring?
| @@ -106,6 +106,19 @@ This should be added both to the LMS and the CMS settings. Instead of a function | |||
| Note that the SCORM XBlock comes with extended S3 storage support out of the box. See the following section: | |||
|
|
|||
| Default storage URL | |||
There was a problem hiding this comment.
This additional setting is not needed we can rely on S3_QUERY_AUTH
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes #99.
A change was introduced in #91 where instead of accessing the url method on the storage backend, we would proxy the assets through the LMS. This was done to generalize the backend and allow for it to work out of the box with external storages such as S3.
However, as pointed out in #99, this can lead to some degraded performance as compared to just accessing scorm files directly from an external storage. Hence, we provide an option to change this behaviour and it is up to the user to choose them.
The default option will be proxying through the LMS as it does not require any extra setup.
An important concept necessary here is that the
urlmethod must be defined on the storage if theDEFAULT_STORAGE_URLoption is set to true.