feat: update search index when course content is updated (TEMP) #645
Conversation
rpenido
force-pushed
the
rpenido/fal-3690-update-search-index-when-course-content-is-changed
branch
from
7e5999b
to
f6d0a54
Compare
rpenido
force-pushed
the
rpenido/fal-3690-update-search-index-when-course-content-is-changed
branch
from
f6d0a54
to
1cbbc18
Compare
bradenmacdonald
force-pushed
the
braden/meilisearch-libraries
branch
3 times, most recently
from
0c43e21
to
6be622e
Compare
| fn(child) | ||
|
|
||
|
|
||
| def rebuild_index(status_cb: Callable[[str], None] | None) -> None: |
There was a problem hiding this comment.
Do we ever need to call this outside of a management command? I didn't think so, which is why I only implemented it as a management command.
There was a problem hiding this comment.
No.
I thought I would reuse some of its code to index a course or a library, but that was not the case.
Are you suggesting moving this back to a management command and leaving in the API only the shared functions?
There was a problem hiding this comment.
Yes, but it's fine either way :)
| # If there is a rebuild in progress, wait for it to finish | ||
| # This could still be a problem if a rebuild starts right after this check, but we don't want to prevent | ||
| # concurrent updates entirely. | ||
| _wait_index_rebuild_lock(INDEX_NAME) |
There was a problem hiding this comment.
What do you think about an alternative approach: if a rebuild is in progress, do the upsert in both the old index and the new index. Then you don't need to use locks or worry as much about race conditions.
There was a problem hiding this comment.
Hmm. The problem is how to safely check that a rebuild is in progress and update atomically without a lock. Could you help me if you see a way?
We could accept some race conditions, and, thinking about it a bit more, the current scenario is fine: if the command is called AFTER the XBLOCK event, the new index will have the correct data (even if it discards the updates from the event because the XBLOCK is already updated).
Or we could stop replacing the index and accept the performance hit to simplify the code.
What do you think @bradenmacdonald ?
There was a problem hiding this comment.
I like the way that you're tracking whether an index update is occurring using the cache. You can stick with that but instead of making it a boolean you can store the name of the new index in that "lock" while the rebuild is happening. Then the normal update code can detect that, and apply the changes to both indexes.
I don't think anything has to be atomic? As long as you make the update in both indexes, it will be fine. Even if the "reindex" command later overwrites it, it will still be the latest version.
| """ | ||
| Create the index for the XBlock | ||
| """ | ||
| # FixMe: Add check for enabled melisearch |
There was a problem hiding this comment.
You can use the new MEILISEARCH_ENABLED setting which I added to my PR.
…ate-search-index-when-course-content-is-changed
…ate-search-index-when-course-content-is-changed
Co-authored-by: Yusuf Musleh <[email protected]>
rpenido
force-pushed
the
rpenido/fal-3690-update-search-index-when-course-content-is-changed
branch
4 times, most recently
from
e0f2562
to
9c051f4
Compare
rpenido
force-pushed
the
rpenido/fal-3690-update-search-index-when-course-content-is-changed
branch
from
34f5722
to
cab3af9
Compare
…ate-search-index-when-course-content-is-changed
rpenido
force-pushed
the
rpenido/fal-3690-update-search-index-when-course-content-is-changed
branch
from
df407c4
to
7320e3c
Compare
rpenido
force-pushed
the
rpenido/fal-3690-update-search-index-when-course-content-is-changed
branch
from
7c82a0b
to
bd28dec
Compare
|
Closes in favor of openedx#34391 |
No description provided.