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

Use index-swap when reindexing #267

Open
brunoocasali opened this issue Jun 20, 2023 · 6 comments
Open

Use index-swap when reindexing #267

brunoocasali opened this issue Jun 20, 2023 · 6 comments
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@brunoocasali
Copy link
Member

brunoocasali commented Jun 20, 2023

Following these issues: #234 and #263

We could improve this method behavior by adding the fix for issue #234 and also make the creation of a new temporary index then adding the data, and then switching it to the current index name using index swap.
So we can run it in production without downtime.

@brunoocasali brunoocasali added enhancement New feature or request good first issue Good for newcomers labels Jun 20, 2023
@brunoocasali brunoocasali changed the title Improvements to the reindex! method Use index-swap when reindexing Aug 3, 2023
@brunoocasali
Copy link
Member Author

This issue is related to this one: meilisearch/integration-guides#223

@ellnix
Copy link
Collaborator

ellnix commented Sep 25, 2023

This presents a couple of difficulties to implement on ms_reindex! on models, primarily because indexes are not 1:1 mapped to models.

The primary issue is about shared indexes. Since currently we only keep track (and can only keep track) of the indexes on models, reindex! does not in fact know what models are on an index.

For the sake of argument imagine a Movie model and a ShortFilm model which have a shared index with uid "Video".

Attempting to keep track of them with some sort of configuration hash is no good. Models in Rails are lazy-loaded, which means the meilisearch do ... end block may not have run for Movie when we call reindex! on ShortFilm so our gem will be blind to the Movie config and swap the entire index with ShortFilm only. While we can reasonably assume that every model will be loaded in time in a production environment, it would be a nasty surprise to someone opening bin/rails console to do some housekeeping to find that calling reindex! on ShortFilm will remove all Movie entries from the "Video" index.

Attempting to cache this on some sort of persistent configuration would likely be vulnerable to edge case bugs with newly added models or updated meilisearch configurations that have not been cached yet.

The current reindex! method does not have to worry about any of this since it does not remove documents from the index (apart from those that fail the if or unless condition).

From the algoliasearch-rails README on shared indexes:

Notes: If you target a single index from several models, you must never use MyModel.reindex and only use MyModel.reindex!. The reindex method uses a temporary index to perform an atomic reindexing: if you use it, the resulting index will only contain records for the current model because it will not reindex the others.

Essentially, they defer judgement about whether or not to do an index swap or a regular reindex to the user. I am personally not sure if I like that idea, it seems to me like it could cause trouble for users who aren't well versed in the terminology or have so many models that it is not feasible to expect them to be accurate in determining if an index is shared or not.

@doutatsu
Copy link

doutatsu commented Dec 13, 2023

Curious if there has been any movement on this? I would really like to reindex without worry (#263 (comment))

@doutatsu
Copy link

doutatsu commented Jun 3, 2024

Bump on this 👁️

@ellnix
Copy link
Collaborator

ellnix commented Jun 4, 2024

Bump on this 👁️

I'm not sure what you want from this issue exactly. I outlined the problem with using index-swap, are you saying that you would want it implemented regardless?

@doutatsu
Copy link

doutatsu commented Jun 4, 2024

Considering the issue hasn't been closed, I assumed it was still up for discussion @ellnix. I do believe not deleting documents as part of re-index is a bug, as it's not a behaviour a user would expect #263 (comment). So whether it's a shared index or something else, I feel like it should be addressed. Otherwise, everyone would have to implement their own "reindex!", that first goes through all the documents to find those that got deleted, delete them and then do another pass re-indexing the existing documents. I feel like the rails integration should be able to handle it for us

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

3 participants