-
Notifications
You must be signed in to change notification settings - Fork 442
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: replace Elasticsearch by Meilisearch #1141
base: sumac
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
- 💥[Feature] Replace Elasticsearch by Meilisearch. Elasticsearch was both a source of complexity and high resource usage. With this change, we no longer run Elasticsearch to perform common search queries across Open edX. This includes: course discovery, courseware search and studio search. Instead, we index all these documents in a Meilisearch instance, which is much more lightweight in terms of memory consumption. (by @regisb) | ||
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -235,7 +235,6 @@ def upgrade_obsolete(config: Config) -> None: | |
for name in [ | ||
"ACTIVATE_LMS", | ||
"ACTIVATE_CMS", | ||
"ACTIVATE_ELASTICSEARCH", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. curious: Why is this not replaced with ACTIVATE_MEILISEARCH? |
||
"ACTIVATE_MONGODB", | ||
"ACTIVATE_MYSQL", | ||
"ACTIVATE_REDIS", | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,10 +1,10 @@ | ||
from __future__ import annotations | ||
|
||
from copy import deepcopy | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why was this moved above? It was correct before in terms of import order. |
||
import os | ||
import re | ||
import shutil | ||
import typing as t | ||
from copy import deepcopy | ||
|
||
import importlib_resources | ||
import jinja2 | ||
|
@@ -56,6 +56,8 @@ def _prepare_environment() -> None: | |
("reverse_host", utils.reverse_host), | ||
("rsa_import_key", utils.rsa_import_key), | ||
("rsa_private_key", utils.rsa_private_key), | ||
("uuid", utils.uuid), | ||
("uid_master_hash", utils.uid_master_hash), | ||
], | ||
) | ||
# Template variables | ||
|
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -34,12 +34,14 @@ | |||
# Behave like memcache when it comes to connection errors | ||||
DJANGO_REDIS_IGNORE_EXCEPTIONS = True | ||||
|
||||
# Elasticsearch connection parameters | ||||
ELASTIC_SEARCH_CONFIG = [{ | ||||
{% if ELASTICSEARCH_SCHEME == "https" %}"use_ssl": True,{% endif %} | ||||
"host": "{{ ELASTICSEARCH_HOST }}", | ||||
"port": {{ ELASTICSEARCH_PORT }}, | ||||
}] | ||||
# Meilisearch connection parameters | ||||
MEILISEARCH_ENABLED = True | ||||
MEILISEARCH_URL = "{{ MEILISEARCH_URL }}" | ||||
MEILISEARCH_PUBLIC_URL = "{{ MEILISEARCH_PUBLIC_URL }}" | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This variable overrides the So I think it should be removed from here, and explicitly set in the lms + cms
Suggested change
|
||||
MEILISEARCH_INDEX_PREFIX = "{{ MEILISEARCH_INDEX_PREFIX }}" | ||||
MEILISEARCH_API_KEY = "{{ MEILISEARCH_API_KEY }}" | ||||
MEILISEARCH_MASTER_KEY = "{{ MEILISEARCH_MASTER_KEY }}" | ||||
SEARCH_ENGINE = "search.meilisearch.MeilisearchEngine" | ||||
|
||||
# Common cache config | ||||
CACHES = { | ||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -37,6 +37,10 @@ | |
"LOCATION": "staticfiles_lms", | ||
} | ||
|
||
# Enable search features | ||
FEATURES["ENABLE_COURSE_DISCOVERY"] = True | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is course-discovery enabled by default in here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed -- I think we only need these feature flags, not
|
||
FEATURES["ENABLE_COURSEWARE_SEARCH"] = True | ||
|
||
# Create folders if necessary | ||
for folder in [DATA_DIR, LOG_DIR, MEDIA_ROOT, STATIC_ROOT, ORA2_FILEUPLOAD_ROOT]: | ||
if not os.path.exists(folder): | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -32,19 +32,18 @@ services: | |
ports: | ||
- "8001:8000" | ||
|
||
meilisearch: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this not behind an IF condition? |
||
ports: | ||
- "127.0.0.1:7700:7700" | ||
networks: | ||
default: | ||
aliases: | ||
- "{{ MEILISEARCH_PUBLIC_URL.split('://')[1] }}" | ||
|
||
# Additional service for watching theme changes | ||
watchthemes: | ||
<<: *openedx-service | ||
command: npm run watch-sass | ||
restart: unless-stopped | ||
|
||
{% if RUN_ELASTICSEARCH and is_docker_rootless() %} | ||
elasticsearch: | ||
ulimits: | ||
memlock: | ||
# Fixes error setting rlimits for ready process in rootless docker | ||
soft: 0 # zero means "unset" in the memlock context | ||
hard: 0 | ||
{% endif %} | ||
|
||
{{ patch("local-docker-compose-dev-services")|indent(2) }} |
Original file line number | Diff line number | Diff line change | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -16,3 +16,7 @@ fi | |||||||||||||
# Create waffle switches to enable some features, if they have not been explicitly defined before | ||||||||||||||
# Copy-paste of units in Studio (highly requested new feature, but defaults to off in Quince) | ||||||||||||||
(./manage.py cms waffle_flag --list | grep contentstore.enable_copy_paste_units) || ./manage.py lms waffle_flag --create contentstore.enable_copy_paste_units --everyone | ||||||||||||||
|
||||||||||||||
# Re-index studio and courseware content | ||||||||||||||
./manage.py cms reindex_studio --experimental | ||||||||||||||
./manage.py cms reindex_course --active | ||||||||||||||
Comment on lines
+21
to
+22
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should gate these too (though not sure about which variable to use to do this?)
Suggested change
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
# Get or create Meilisearch API key | ||
python -c " | ||
import meilisearch | ||
client = meilisearch.Client('{{ MEILISEARCH_URL }}', '{{ MEILISEARCH_MASTER_KEY }}') | ||
try: | ||
client.get_key('{{ MEILISEARCH_API_KEY_UID }}') | ||
print('Key already exists') | ||
except meilisearch.errors.MeilisearchApiError: | ||
print('Key does not exist: creating...') | ||
client.create_key({ | ||
'name': 'Open edX backend API key', | ||
'uid': '{{ MEILISEARCH_API_KEY_UID }}', | ||
'actions': ['*'], | ||
'indexes': ['{{ MEILISEARCH_INDEX_PREFIX }}*'], | ||
'expiresAt': None, | ||
'description': 'Use it for backend API calls -- Created by Tutor', | ||
}) | ||
" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this wont work out of box with discovery, right? course-discovery uses ES heavily in various places. Merging this can break certain things on tutor-discovery.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmed -- trying to run this with
tutor-discovery
enabled throws an Elasticsearch connection error on./manage.py update_index --disable-change-limit
(after working around some missing variables).We're hoping to deprecate Course Discovery someday, so maybe we can move the elasticsearch dependency and setup to tutor-discovery? Or to its own plugin?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created overhangio/tutor-discovery#88 to add ES to tutor-discovery and unblock this PR.