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

Fix exploded elasticsearch import #2963

Merged
merged 4 commits into from
Mar 6, 2022

Conversation

tiendo1011
Copy link
Contributor

@tiendo1011 tiendo1011 commented Feb 19, 2022

Right now, running bundle exec rake environment elasticsearch:import:all DIR=app/models FORCE=y to setup the database as suggestion from this guide throw this error:

ArgumentError: wrong number of arguments (given 1, expected 0)
ruby/3.0.3/lib/ruby/gems/3.0.0/gems/activerecord-6.1.4.6/lib/active_record/relation/batches.rb:128:in `find_in_batches'

It is caused by elasticsearch-rails gem, and is fixed in this PR, I upgrade the gem to fix this issue.

@simi
Copy link
Member

simi commented Feb 19, 2022

@tiendo1011 can you please explain why is this change needed?

@tiendo1011
Copy link
Contributor Author

Hi @simi, sr I meant this to be a WIP one, let me update the description for it, too.

@tiendo1011 tiendo1011 changed the title Fix exploded elasticsearch import [wip] Fix exploded elasticsearch import Feb 20, 2022
@tiendo1011 tiendo1011 force-pushed the fix-exploded-elasticsearch-import branch from cf733a2 to 08140b3 Compare February 26, 2022 06:16
@tiendo1011 tiendo1011 changed the title [wip] Fix exploded elasticsearch import Fix exploded elasticsearch import Feb 26, 2022
@simi
Copy link
Member

simi commented Feb 28, 2022

Any idea about the CI problem?

@tiendo1011
Copy link
Contributor Author

tiendo1011 commented Mar 3, 2022

@simi it seems to me that it's from this method inside elasticsearch-model gem:

def delete_index!(options={})
  target_index = options.delete(:index) || self.index_name

  begin
    self.client.indices.delete index: target_index
  rescue Exception => e
    if e.class.to_s =~ /NotFound/ && options[:force]
      client.transport.transport.logger.debug("[!!!] Index does not exist (#{e.class})") if client.transport.transport.logger
      nil
    else
      raise e
    end
  end
end

Notice the call in client.transport.transport, it's where the error NoMethodError: undefined method 'transport' originated from.

The reason for this is that the client inside that method expect an ElasticSearch::Client instance, but from out setup in #config/initializers/elasticsearch.rb:27, the client is set to an Elasticsearch::Transport::Client instance (this is already the case without the changes in this PR).

I guess there is some mismatch here, is there a special reason that we need to set client to Elasticsearch::Transport::Client instead of ElasticSearch::Client?

@simi
Copy link
Member

simi commented Mar 5, 2022

Can you try to do bundle update elasticsearch and push? That fixes problems for me locally. I can run both elasticsearch:import:all and tests.

@tiendo1011
Copy link
Contributor Author

tiendo1011 commented Mar 5, 2022

@simi sure thing. I've done it.

@simi
Copy link
Member

simi commented Mar 5, 2022

It seems super effective :)

Can you fix the rubocop problem? https://github.com/rubygems/rubygems.org/runs/5432855707?check_suite_focus=true Once done I think we're ready to merge.

@tiendo1011
Copy link
Contributor Author

@simi nice :), I've fixed the rubocop problem

@simi simi merged commit c45be12 into rubygems:master Mar 6, 2022
@simi
Copy link
Member

simi commented Mar 6, 2022

@tiendo1011 💚

sonalkr132 added a commit that referenced this pull request Mar 7, 2022
…iguration (#2963)"

This reverts commit c45be12.
Fixes:

Elasticsearch::UnsupportedProductError: The client noticed that the
server is not a supported distribution of Elasticsearch.

https://app.honeybadger.io/projects/40972/faults/84396370
@sonalkr132
Copy link
Member

Hi, this was reverted f78892a
I don't plan on updating ES as of now. I would recommend using Rubygem.__elasticsearch__.import(force: true) from console and check if that works.

@simi
Copy link
Member

simi commented Mar 7, 2022

@sonalkr132 sad to hear. Any idea why specs were ok with this change? Which version of ES are we running at staging and which in production?

@sonalkr132
Copy link
Member

I may have been too quick to say it was an update issue. Google the exception I added in revert commit. Looks like elasticsearch is intentionally breaking support for opensearch (aws).

Any idea why specs were ok with this change?

We may need to migrate to https://opendistro.github.io/for-elasticsearch/

@simi
Copy link
Member

simi commented Mar 8, 2022

@sonalkr132 would be great start to upgrade docker-compose to use https://hub.docker.com/r/amazon/opendistro-for-elasticsearch?

@sonalkr132
Copy link
Member

Someone needs to research this a bit. I am not sure if opensearch managed is using same docker image. opendistro and opensearch are two separate things(?). Ideally we should migrate to opesearch client as well at the same time.

@simi
Copy link
Member

simi commented Mar 14, 2022

@sonalkr132 Any idea which version of OpenSearch is used on staging/production?

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.

3 participants