Fix race condition in queued record & document removal #290
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #290 +/- ##
==========================================
+ Coverage 89.15% 89.32% +0.17%
==========================================
Files 10 11 +1
Lines 664 684 +20
==========================================
+ Hits 592 611 +19
- Misses 72 73 +1 ☔ View full report in Codecov by Sentry. |
ellnix
force-pushed
the
fix-race-condition-in-queued-removal
branch
from
4804396 to
663c9a9
Compare
ellnix
force-pushed
the
fix-race-condition-in-queued-removal
branch
from
7d17cb1 to
967ccb2
Compare
ellnix
force-pushed
the
fix-race-condition-in-queued-removal
branch
from
9d1fee3 to
f42758d
Compare
| documents.each do |document| | ||
| index = MeiliSearch::Rails.client.index(document[:index_uid]) | ||
|
|
||
| if document[:synchronous] |
There was a problem hiding this comment.
Is it helpful to do the check instead of just calling async or sync. But I changed my mind 😅. Let's let the user decide; we don't know the use cases. Maybe it is too much to wait for a call in certain apps, but indispensable for others...
spec/ms_clean_up_job_spec.rb
Outdated
| it 'removes record from all indexes' do | ||
| indexes.each(&:delete_all_documents) | ||
|
|
||
| record |
There was a problem hiding this comment.
I need to say that I think just calling the creation of the AR record like this is weird to me, the line seems lost when I read the whole suite 😅
There was a problem hiding this comment.
A let! wouldn't do the trick?
There was a problem hiding this comment.
A let would run too soon, the record would be created and then indexes.each(&:delete_all_documents) would run, which would cause the ms documents to be created and then promptly deleted.
I'll see if I can refactor it to look less stupid xD
There was a problem hiding this comment.
I attempted to refactor it, let me know if I should try again.
The signature of MSJob was not suitable for a fix to the race condition between MSJob and active_record#destroy.
ellnix
force-pushed
the
fix-race-condition-in-queued-removal
branch
from
40d609b to
91437f1
Compare
|
Would be great to get this one merged! This is causing us some issues. |
|
bors merge |
Pull Request
Related issue
Fixes #266
What does this PR do?
The purpose of this PR is to detach the MeiliSearch document deletion process from the ActiveRecord object so that documents corresponding to a record can be deleted even if the record no longer exists in the database.
Tests were also added for new functionality and to hopefully prevent regressions.
PR checklist
Please check if your PR fulfills the following requirements:
Solution Considerations
Please read #266 for full context:
MSJob(although I suspect it would be better if it was replaced with a job dedicated to enqueued indexing, since that's all it does now anyway)ActiveJobneeds to somehow serialize the parameters to a job, default serialization (throughGlobalID) relies on a database entry to reinitialize the record when the job is runms_entriesmethods whose purpose is to use the current configuration to list every index the record may have a document in, along with the record's primary key in that indexUtilities.indexable?so it is possible that some of the entries it returns are not actually in MeiliSearch, this is by design since its intention is to be exhaustive and in theory those primary keys cannot belong to another record so deleting them should be ok