Skip to content

fix: update library index synchronously [FC-0062] #35367

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.

Sign up for GitHub

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

Merged
bradenmacdonald merged 1 commit into from
Aug 30, 2024
Merged

fix: update library index synchronously [FC-0062] #35367

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion openedx/core/djangoapps/content/search/handlers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,13 @@ def content_library_updated_handler(**kwargs) -> None:
log.error("Received null or incorrect data for event")
return

update_content_library_index_docs.delay(str(content_library_data.library_key))
# Update content library index synchronously to make sure that search index is updated before
# the frontend invalidates/refetches index.
# Currently, this is only required to make sure that removed/discarded components are removed
# from the search index and displayed to user properly. If it becomes a performance bottleneck
# for other update operations other than discard, we can update CONTENT_LIBRARY_UPDATED event
# to include a parameter which can help us decide if the task needs to run sync or async.

@bradenmacdonald bradenmacdonald Aug 26, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Or, we can keep it async and create some signal to send to the frontend once the index has been fully updated. (or allow the frontend to poll the task status)

I am a little concerned about the performance of this, if it's happening when we publish changes. Discarding changes or renaming the library are rare operations, but publishing will happen frequently and the reindex could make that slow if there are a lot of blocks in the library. Maybe we just need to test it and see though.

I would prefer that we just update the actual blocks in the index that were discarded/published - I don't suppose we have that data anywhere?

@rpenido rpenido Aug 29, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@bradenmacdonald

Should we prevent the user from seeing/handling the outdated components? If this is the case, we need to block the interface, and it doesn't make much difference if the process is sync or async + signal frontend-wise, right?

This pattern will happen across the library authoring (from adding/removing content to tagging) as we use meilisearch as the source.

I don't see a better way than running these operations sync and working on optimizing the bottlenecks.

Also, this PR is only for one event, and we need to tackle it in some other places. Should we create a task to discuss and implement the solution for this?

We could also add more granularity to the events (as discussed in the other PR) to handle better what should be done.

@bradenmacdonald bradenmacdonald Aug 30, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm fine with this for now.

Should we prevent the user from seeing/handling the outdated components? If this is the case, we need to block the interface, and it doesn't make much difference if the process is sync or async + signal frontend-wise, right?

Yes, you're right.

This pattern will happen across the library authoring (from adding/removing content to tagging) as we use meilisearch as the source.

No; in almost all other cases, we only synchronously update one document when there is a change. We're not re-indexing the whole course or whole library. So it's synchronous but it's extremely fast.

We could also add more granularity to the events (as discussed in the other PR) to handle better what should be done.

I think that's the best long-term solution.

Should we create a task to discuss and implement the solution for this?

Yes please.

@bradenmacdonald bradenmacdonald Aug 30, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

it doesn't make much difference if the process is sync or async + signal frontend-wise, right?

It does matter if we're tying up a django web worker thread for a long period of time - it could slow down the site for other people. Putting it into an async task + signal solves that. But for now I believe this is fast enough that it won't be a problem.

update_content_library_index_docs.apply(args=[str(content_library_data.library_key)])


@receiver(CONTENT_OBJECT_TAGS_CHANGED)
Expand Down