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.
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
feat: replace Elasticsearch by Meilisearch #1141
Changes from all commits
ebc39e3
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Should gate these too (though not sure about which variable to use to do this?)
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.
No, I don't think that's right. We assume that meilisearch is running, somewhere. Even if RUN_MEILISEARCH is false, it just means that meilisearch is running on a remote host. So we still need to run the migration commands.
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.
Oh yes, you're correct!
There's an issue with
reindex_course --setup
referencing elasticsearch directly (openedx/edx-platform#35743), but that's not Tutor's problem :)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.
I suggest we run
./manage.py lms shell -c "import search.meilisearch; search.meilisearch.create_indexes()"
in init/lms.sh. What do you think?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.
Also keep in mind that until we resolve openedx/modular-learning#235 (which is in progress), we should avoid running
./manage.py cms reindex_studio --experimental
on any instances with thousands of courses or more.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.
As for re-indexing courses, I haven't looked into it but it does sound like we're doing it more than is necessary.
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.
Is there a function that would only create and configure the indexes (without deleting them first), such that we could call this function at every launch? Pretty much like a
./manage.py migrate
command.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.
It doesn't exist yet, but we could add that. A
--create-only
flag would be trivial.The edge case is when upgrading to a newer commit/release and the index parameters have changed; in that case, we need to replace the index and the new index will be empty unless we also re-index all the content. This could be done asynchronously though; it also doesn't replace the existing index until the reindexing is complete, so there's no downtime.
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.
I think we do need a lightweight, idempotent index creation command that works in just all cases. This can take different forms: a single python function, a management command, or a set of migrations. If we can get this in Sumac then that's great, if not then we should probably think about it for later releases.
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.
I added a proposal to openedx/modular-learning#235 for a new
--init
flag that will achieve this.