Skip to content
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: update search index when course content is updated (TEMP) #645

Conversation

rpenido
Copy link
Member

@rpenido rpenido commented Mar 14, 2024

No description provided.

@rpenido rpenido force-pushed the rpenido/fal-3690-update-search-index-when-course-content-is-changed branch from 7e5999b to f6d0a54 Compare March 14, 2024 19:00
@rpenido rpenido force-pushed the rpenido/fal-3690-update-search-index-when-course-content-is-changed branch from f6d0a54 to 1cbbc18 Compare March 14, 2024 19:54
@bradenmacdonald bradenmacdonald force-pushed the braden/meilisearch-libraries branch 3 times, most recently from 0c43e21 to 6be622e Compare March 18, 2024 02:54
fn(child)


def rebuild_index(status_cb: Callable[[str], None] | None) -> None:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done: d63195d

"""
Create the index for the XBlock
"""
# FixMe: Add check for enabled melisearch
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use the new MEILISEARCH_ENABLED setting which I added to my PR.

@rpenido 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 March 20, 2024 21:21
@rpenido rpenido force-pushed the rpenido/fal-3690-update-search-index-when-course-content-is-changed branch from 34f5722 to cab3af9 Compare March 20, 2024 21:52
@rpenido rpenido force-pushed the rpenido/fal-3690-update-search-index-when-course-content-is-changed branch from df407c4 to 7320e3c Compare March 22, 2024 15:10
@rpenido rpenido force-pushed the rpenido/fal-3690-update-search-index-when-course-content-is-changed branch from 7c82a0b to bd28dec Compare March 22, 2024 15:50
@rpenido
Copy link
Member Author

rpenido commented Mar 26, 2024

Closes in favor of openedx#34391

@rpenido rpenido closed this Mar 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants