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

ES 8.x upgrade #917

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 3 additions & 2 deletions chewy.gemspec
Expand Up @@ -11,13 +11,14 @@ Gem::Specification.new do |spec|
spec.description = 'Chewy provides functionality for Elasticsearch index handling, documents import mappings and chainable query DSL'
spec.homepage = 'https://github.com/toptal/chewy'
spec.license = 'MIT'
spec.required_ruby_version = '~> 3.0'

spec.files = `git ls-files`.split($RS)
spec.executables = spec.files.grep(%r{^bin/}) { |f| File.basename(f) }
spec.require_paths = ['lib']

spec.add_dependency 'activesupport', '>= 5.2' # Remove with major version bump, 8.x
spec.add_dependency 'elasticsearch', '>= 7.12.0', '< 7.14.0'
spec.add_dependency 'activesupport', '>= 6.1'
spec.add_dependency 'elasticsearch', '>= 8.11', '< 9.0'
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the plan here to do a major version bump of Chewy as well? So the chewy 7.x series will continue to get any bugfixes for ES7 gems, and the chewy 8.x series will support the ES8.x gems?

Copy link
Contributor Author

@konalegi konalegi Dec 16, 2023

Choose a reason for hiding this comment

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

Is the plan here to do a major version bump of Chewy as well?

Yes, since the major version of the Chewy, is equal to major version of the ES.

So the chewy 7.x series will continue to get any bugfixes for ES7 gems, and the chewy 8.x series will support the ES8.x gems?

This my plan, but since I have never done this before, cannot say for sure, how hard it will be to support both versions at the same time.

Choose a reason for hiding this comment

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

Hopefully it shouldn't be too much work to keep the 7.x branch up to date with changes in the Elasticsearch Ruby client. The last minor release is 7.17 and we're not planning to release any new minors or add any new features for the moment, just addressing bugs or security issues, so patch releases only.

spec.add_dependency 'elasticsearch-dsl'
spec.metadata['rubygems_mfa_required'] = 'true'
end
16 changes: 16 additions & 0 deletions docker-compose.yml
@@ -0,0 +1,16 @@
version: "3.4"
services:
elasticsearch_test:
image: "elasticsearch:8.11.3"
environment:
- bootstrap.memory_lock=${ES_MEMORY_LOCK:-false}
- "ES_JAVA_OPTS=-Xms${TEST_ES_HEAP_SIZE:-500m} -Xmx${TEST_ES_HEAP_SIZE:-500m}"
- discovery.type=single-node
- xpack.security.enabled=false
- action.destructive_requires_name=false
ports:
- "127.0.0.1:9250:9200"
ulimits:
nofile:
soft: 65536
hard: 65536
10 changes: 5 additions & 5 deletions lib/chewy/index/actions.rb
Expand Up @@ -32,7 +32,7 @@ def exists?
#
def create(*args, **kwargs)
create!(*args, **kwargs)
rescue Elasticsearch::Transport::Transport::Errors::BadRequest
rescue Elastic::Transport::Transport::Errors::BadRequest
false
end

Expand Down Expand Up @@ -83,9 +83,9 @@ def delete(suffix = nil)
result = client.indices.delete index: index_names.join(',')
Chewy.wait_for_status if result
result
# es-ruby >= 1.0.10 handles Elasticsearch::Transport::Transport::Errors::NotFound
# es-ruby >= 1.0.10 handles Elastic::Transport::Transport::Errors::NotFound
# by itself, rescue is for previous versions
rescue Elasticsearch::Transport::Transport::Errors::NotFound
rescue Elastic::Transport::Transport::Errors::NotFound
false
end

Expand All @@ -99,9 +99,9 @@ def delete(suffix = nil)
# UsersIndex.delete '01-2014' # deletes `users_01-2014` index
#
def delete!(suffix = nil)
# es-ruby >= 1.0.10 handles Elasticsearch::Transport::Transport::Errors::NotFound
# es-ruby >= 1.0.10 handles Elastic::Transport::Transport::Errors::NotFound
# by itself, so it is raised here
delete(suffix) or raise Elasticsearch::Transport::Transport::Errors::NotFound
delete(suffix) or raise Elastic::Transport::Transport::Errors::NotFound
end

# Deletes and recreates index. Supports suffixes.
Expand Down
2 changes: 1 addition & 1 deletion lib/chewy/index/aliases.rb
Expand Up @@ -22,7 +22,7 @@ def aliases

def empty_if_not_found
yield
rescue Elasticsearch::Transport::Transport::Errors::NotFound
rescue Elastic::Transport::Transport::Errors::NotFound
[]
end
end
Expand Down
2 changes: 1 addition & 1 deletion lib/chewy/index/syncer.rb
Expand Up @@ -213,7 +213,7 @@ def outdated_sync_field_type
@outdated_sync_field_type = mappings
.fetch('properties', {})
.fetch(@index.outdated_sync_field.to_s, {})['type']
rescue Elasticsearch::Transport::Transport::Errors::NotFound
rescue Elastic::Transport::Transport::Errors::NotFound
nil
end
end
Expand Down
4 changes: 2 additions & 2 deletions lib/chewy/search/request.rb
Expand Up @@ -854,7 +854,7 @@ def count
else
Chewy.client.count(only(WHERE_STORAGES).render)['count']
end
rescue Elasticsearch::Transport::Transport::Errors::NotFound
rescue Elastic::Transport::Transport::Errors::NotFound
0
end

Expand Down Expand Up @@ -1035,7 +1035,7 @@ def perform(additional = {})
request_body = render.merge(additional)
ActiveSupport::Notifications.instrument 'search_query.chewy', notification_payload(request: request_body) do
Chewy.client.search(request_body)
rescue Elasticsearch::Transport::Transport::Errors::NotFound
rescue Elastic::Transport::Transport::Errors::NotFound
{}
end
end
Expand Down
16 changes: 8 additions & 8 deletions spec/chewy/index/actions_spec.rb
Expand Up @@ -78,12 +78,12 @@
specify do
expect do
DummiesIndex.create!
end.to raise_error(Elasticsearch::Transport::Transport::Errors::BadRequest).with_message(/already exists.*dummies/)
end.to raise_error(Elastic::Transport::Transport::Errors::BadRequest).with_message(/already exists.*dummies/)
end
specify do
expect do
DummiesIndex.create!('2013')
end.to raise_error(Elasticsearch::Transport::Transport::Errors::BadRequest).with_message(/Invalid alias name \[dummies\]/)
end.to raise_error(Elastic::Transport::Transport::Errors::BadRequest).with_message(/Invalid alias name \[dummies\]/)
end
end

Expand All @@ -100,7 +100,7 @@
specify do
expect do
DummiesIndex.create!('2013')
end.to raise_error(Elasticsearch::Transport::Transport::Errors::BadRequest).with_message(/already exists.*dummies_2013/)
end.to raise_error(Elastic::Transport::Transport::Errors::BadRequest).with_message(/already exists.*dummies_2013/)
end
specify { expect(DummiesIndex.create!('2014')['acknowledged']).to eq(true) }

Expand Down Expand Up @@ -190,11 +190,11 @@
end

describe '.delete!' do
specify { expect { DummiesIndex.delete! }.to raise_error(Elasticsearch::Transport::Transport::Errors::NotFound) }
specify { expect { DummiesIndex.delete! }.to raise_error(Elastic::Transport::Transport::Errors::NotFound) }
specify do
expect do
DummiesIndex.delete!('2013')
end.to raise_error(Elasticsearch::Transport::Transport::Errors::NotFound)
end.to raise_error(Elastic::Transport::Transport::Errors::NotFound)
end

context do
Expand Down Expand Up @@ -768,7 +768,7 @@
.to receive(:clear_cache)
.and_call_original
expect { CitiesIndex.clear_cache({index: unexisted_index_name}) }
.to raise_error Elasticsearch::Transport::Transport::Errors::NotFound
.to raise_error Elastic::Transport::Transport::Errors::NotFound
end
end

Expand Down Expand Up @@ -820,7 +820,7 @@
.to receive(:reindex)
.and_call_original
expect { CitiesIndex.reindex(source: unexisting_index, dest: dest_index_with_prefix) }
.to raise_error Elasticsearch::Transport::Transport::Errors::NotFound
.to raise_error Elastic::Transport::Transport::Errors::NotFound
end
end

Expand Down Expand Up @@ -883,7 +883,7 @@
context 'index name' do
specify do
expect { CitiesIndex.update_mapping(unexisting_index, body_hash) }
.to raise_error Elasticsearch::Transport::Transport::Errors::NotFound
.to raise_error Elastic::Transport::Transport::Errors::NotFound
end
end

Expand Down
2 changes: 1 addition & 1 deletion spec/chewy/index/import/bulk_builder_spec.rb
Expand Up @@ -216,7 +216,7 @@ def derived
end

def do_raw_index_comment(options:, data:)
CommentsIndex.client.index(options.merge(index: 'comments', type: '_doc', refresh: true, body: data))
CommentsIndex.client.index(options.merge(index: 'comments', refresh: true, body: data))
end

def raw_index_comment(comment)
Expand Down
28 changes: 14 additions & 14 deletions spec/chewy/index/import_spec.rb
Expand Up @@ -204,18 +204,18 @@ def subscribe_notification
end
end

let(:mapper_parsing_exception) do
let(:document_parsing_exception) do
{
'type' => 'mapper_parsing_exception',
'reason' => 'object mapping for [name] tried to parse field [name] as object, but found a concrete value'
'type' => 'document_parsing_exception',
'reason' => '[1:9] object mapping for [name] tried to parse field [name] as object, but found a concrete value'
}
end

specify do
payload = subscribe_notification
import dummy_cities, batch_size: 2
expect(payload).to eq(index: CitiesIndex,
errors: {index: {mapper_parsing_exception => %w[1 2 3]}},
errors: {index: {document_parsing_exception => %w[1 2 3]}},
import: {index: 3})
end
end
Expand Down Expand Up @@ -270,8 +270,8 @@ def subscribe_notification
expect(payload).to eq(
errors: {
index: {{
'type' => 'mapper_parsing_exception',
'reason' => 'object mapping for [object] tried to parse field [object] as object, but found a concrete value'
'type' => 'document_parsing_exception',
'reason' => '[1:27] object mapping for [object] tried to parse field [object] as object, but found a concrete value'
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these numeric prefixes ([1:27] in this example) new error-code constants from the ES client gem ... or contingent on the spec data setup here, or something else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't know this yet, I've just made all specs passing, to see how many things do we break by upgrading to ES 8.x

Copy link

Choose a reason for hiding this comment

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

When can we expect for this to be considered for release?

} => %w[2 4]}
},
import: {index: 6},
Expand All @@ -293,8 +293,8 @@ def subscribe_notification
expect(payload).to eq(
errors: {
index: {{
'type' => 'mapper_parsing_exception',
'reason' => 'object mapping for [object] tried to parse field [object] as object, but found a concrete value'
'type' => 'document_parsing_exception',
'reason' => '[1:27] object mapping for [object] tried to parse field [object] as object, but found a concrete value'
} => %w[2 4]}
},
import: {index: 6},
Expand All @@ -319,8 +319,8 @@ def subscribe_notification
expect(payload).to eq(
errors: {
index: {{
'type' => 'mapper_parsing_exception',
'reason' => 'object mapping for [object] tried to parse field [object] as object, but found a concrete value'
'type' => 'document_parsing_exception',
'reason' => '[1:27] object mapping for [object] tried to parse field [object] as object, but found a concrete value'
} => %w[2 4]}
},
import: {index: 6},
Expand Down Expand Up @@ -383,8 +383,8 @@ def subscribe_notification

# Full match doesn't work here.
expect(payload[:errors][:update].keys).to match([
hash_including('type' => 'document_missing_exception', 'reason' => '[_doc][1]: document missing'),
hash_including('type' => 'document_missing_exception', 'reason' => '[_doc][3]: document missing')
hash_including('type' => 'document_missing_exception', 'reason' => '[1]: document missing'),
hash_including('type' => 'document_missing_exception', 'reason' => '[3]: document missing')
])
expect(payload[:errors][:update].values).to eq([['1'], ['3']])
expect(imported_cities).to match_array([
Expand Down Expand Up @@ -431,8 +431,8 @@ def subscribe_notification
expect(payload).to eq(
errors: {
update: {{
'type' => 'mapper_parsing_exception',
'reason' => 'object mapping for [object] tried to parse field [object] as object, but found a concrete value'
'type' => 'document_parsing_exception',
'reason' => '[1:26] object mapping for [object] tried to parse field [object] as object, but found a concrete value'
} => %w[2 4]}
},
import: {index: 6},
Expand Down
3 changes: 0 additions & 3 deletions spec/chewy/index/specification_spec.rb
Expand Up @@ -46,7 +46,6 @@
specify do
expect { specification1.lock! }.to change { Chewy::Stash::Specification.all.hits }.from([]).to([{
'_index' => 'chewy_specifications',
'_type' => '_doc',
'_id' => 'places',
'_score' => 1.0,
'_source' => {'specification' => Base64.encode64({
Expand All @@ -62,7 +61,6 @@
specify do
expect { specification5.lock! }.to change { Chewy::Stash::Specification.all.hits }.to([{
'_index' => 'chewy_specifications',
'_type' => '_doc',
'_id' => 'places',
'_score' => 1.0,
'_source' => {'specification' => Base64.encode64({
Expand All @@ -71,7 +69,6 @@
}.to_json)}
}, {
'_index' => 'chewy_specifications',
'_type' => '_doc',
'_id' => 'namespace/cities',
'_score' => 1.0,
'_source' => {'specification' => Base64.encode64({
Expand Down
4 changes: 2 additions & 2 deletions spec/chewy/runtime_spec.rb
Expand Up @@ -3,7 +3,7 @@
describe Chewy::Runtime do
describe '.version' do
specify { expect(described_class.version).to be_a(described_class::Version) }
specify { expect(described_class.version).to be >= '7.0' }
specify { expect(described_class.version).to be < '8.0' }
specify { expect(described_class.version).to be >= '8.0' }
specify { expect(described_class.version).to be < '9.0' }
end
end
2 changes: 1 addition & 1 deletion spec/chewy/search/response_spec.rb
Expand Up @@ -39,7 +39,7 @@
specify { expect(subject.hits).to all be_a(Hash) }
specify do
expect(subject.hits.flat_map(&:keys).uniq)
.to match_array(%w[_id _index _type _score _source sort])
.to match_array(%w[_id _index _score _source sort])
end

context do
Expand Down
2 changes: 1 addition & 1 deletion spec/chewy_spec.rb
Expand Up @@ -108,7 +108,7 @@
expect(CitiesIndex.exists?).to eq true
expect(PlacesIndex.exists?).to eq true

expect { Chewy.create_indices! }.to raise_error(Elasticsearch::Transport::Transport::Errors::BadRequest)
expect { Chewy.create_indices! }.to raise_error(Elastic::Transport::Transport::Errors::BadRequest)
end
end
end