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

Allow model saving to pass even if MeiliSearch instance is offline #187

Open
mech opened this issue Sep 8, 2022 · 8 comments
Open

Allow model saving to pass even if MeiliSearch instance is offline #187

mech opened this issue Sep 8, 2022 · 8 comments
Labels
needs investigation Needs to take time to understand the issue.

Comments

@mech
Copy link

mech commented Sep 8, 2022

Hi, I have a use-case where I want my model to still be able to perform basic saving to database even if MeiliSearch instance is not started or found.

I don't think setting the raise_on_failure to false is the correct approach if I understand it correctly.

Specifically I am hitting this exception whenever I want to save my ActiveRecord model.

MeiliSearch::CommunicationError: An error occurred while trying to connect to the MeiliSearch instance: Failed to open TCP connection to localhost:7700 (Connection refused - connect(2) for 127.0.0.1:7700)

I understand that this is due to server not started, but I think this is quite a common situation and normal Rails persistent operation should still pass without failure even if MeiliSearch is not online. Is this possible?

@brunoocasali brunoocasali added the needs investigation Needs to take time to understand the issue. label Sep 8, 2022
@brunoocasali
Copy link
Member

Hi @mech, I'm sorry but I couldn't investigate this issue yet.

I did not forget about it, I just need some time to make it a priority. Hope you will understand!

Take care,

@mech
Copy link
Author

mech commented Sep 23, 2022

Wow no worry, everyone has their time constraint and priority. Cheer 😀 and happy weekend!

@macwilko
Copy link

@mech I had this same problem, the way I solved it was the following:

  after_touch do
    if Rails.env.production?
      begin
        index!
      rescue => exception
        NotifyInDiscordJob.perform_async("message" => "Unable to index a Customer #{exception.message}")
      end
    end
  end

@ellnix
Copy link
Collaborator

ellnix commented Sep 29, 2023

I cannot reproduce this issue. Indeed this error:

MeiliSearch::CommunicationError: An error occurred while trying to connect to the MeiliSearch instance: Failed to open TCP connection to localhost:7700 (Connection refused - connect(2) for 127.0.0.1:7700)

does appear but it does not prevent the object from being saved. The callback that is causing this error is an after_commit (or after_save)

            if respond_to?(:after_commit)
              after_commit :ms_perform_index_tasks
            elsif respond_to?(:after_save)
              after_save :ms_perform_index_tasks
            end

Which means that the record will have been persisted before this error occurs.

@brunoocasali
Copy link
Member

So maybe the issue is the error is being raised is actually misleading the user @ellnix?

@ellnix
Copy link
Collaborator

ellnix commented Oct 9, 2023

So maybe the issue is the error is being raised is actually misleading the user @ellnix?

Maybe, but this is a meilisearch-ruby error, and I cannot see an alternative to throwing an exception if you fail to connect to your configured MeiliSearch instance.

Another alternative could be to somehow queue all indexing operations and run them when the MeiliSearch instance comes back online. I do not think there is a way we could "wait" for the instance to be back online, but maybe the auto index operation, when it succeeds, could check for tasks that were queued while the search instance was offline.

So something like

Untitled Diagram drawio(1)

The main problem being finding some way to persist tasks, especially since this gem does not necessitate a background queue (and even if it did, most ActiveJob backends are in-memory).

This might be of some small use in production, in case your meilisearch instance is temporarily down and in the meantime there are updates to it that need to happen. In that case you wouldn't have to reindex models with millions of records.

But other than that I can't think of a use case for this in production.

@brunoocasali
Copy link
Member

If we could have a way to opt-in to this feature, it would be an excellent addition.
The issue is, like you said, the gem does not require a background queue, and I think it should stay like this.

But thinking more about it, if the user already uses the built-in feature https://github.com/meilisearch/meilisearch-rails/blob/main/README.md#queues--background-jobs I don't think we should even consider the health of meilisearch, because the system will enqueue the index by default, and retry the job if needed.

So I guess there is no need to handle this condition at all right?

@ellnix
Copy link
Collaborator

ellnix commented Oct 13, 2023

But thinking more about it, if the user already uses the built-in feature https://github.com/meilisearch/meilisearch-rails/blob/main/README.md#queues--background-jobs I don't think we should even consider the health of meilisearch, because the system will enqueue the index by default, and retry the job if needed.

The problem is that we could have a race condition in this situation where if we have two actions A and B, and A was queued before B it might fail, be retried, and run after B if the instance came back online between A and B. Think of a situation like this:

      it 'will stay deleted' do
        record = { objectId: 123,  title: 'Pride and Prejudice', comment: 'A great book' }
        index.add_documents!(record)

        # instance goes offline
        # user updates record
        record[:title] = 'Crime and Punishment'

        # ActiveJob background task fails and will be retried in 3 seconds
        t = Thread.new do
          sleep 3
          index.update_documents!(record)
        end

        # Instance comes back online
        # user deletes record
        index.delete_document!(123)
        t.join

        expect(index.document(123)).to be_nil
      end

which results in

     Failure/Error: expect(index.document(123)).to be_nil
     
       expected: nil
            got: {"comment"=>"A great book", "objectId"=>123, "title"=>"Crime and Punishment"}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs investigation Needs to take time to understand the issue.
Projects
None yet
Development

No branches or pull requests

4 participants