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

Add support for elasticsearch v2.0...v2.1 #297

Merged
merged 10 commits into from
Dec 17, 2015
Merged

Conversation

sergeygaychuk
Copy link
Contributor

Tested on:

  1. v1.7.3
  2. v2.0
  3. v2.1
    a) with delete-by-query plugin
    b) without delete-by-query plugin

Maybe someone can setup v2.x version on Travis CI.

Signed-off-by: Sergey Gaichuk <sergey.gaychuk@gmail.com>
Signed-off-by: Sergey Gaichuk <sergey.gaychuk@gmail.com>
Signed-off-by: Sergey Gaichuk <sergey.gaychuk@gmail.com>
Signed-off-by: Sergey Gaichuk <sergey.gaychuk@gmail.com>
Signed-off-by: Sergey Gaichuk <sergey.gaychuk@gmail.com>
Signed-off-by: Sergey Gaichuk <sergey.gaychuk@gmail.com>
Signed-off-by: Sergey Gaichuk <sergey.gaychuk@gmail.com>
Signed-off-by: Sergey Gaichuk <sergey.gaychuk@gmail.com>
Signed-off-by: Sergey Gaichuk <sergey.gaychuk@gmail.com>
Signed-off-by: Sergey Gaichuk <sergey.gaychuk@gmail.com>
@@ -21,7 +21,7 @@
let(:range) { (time - 1.minute)..(time + 1.minute) }

specify { expect(PostsIndex.total).to eq(3) }
specify { expect(PostsIndex.filter { published_at == o{range} }.count).to eq(2) }
specify { expect(PostsIndex.filter { published_at == o{range.min.utc..range.max.utc} }.count).to eq(2) }
specify { expect(PostsIndex.filter { published_at == o{[range.min.to_date..range.max.to_date]} }.count).to eq(1) }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have some troubles with that line on v2.x. I'll open issue on elastic.

@pyromaniac
Copy link
Contributor

Wow wow!

@@ -992,7 +997,7 @@ def _response
begin
Chewy.client.search(_request)
rescue Elasticsearch::Transport::Transport::Errors::NotFound => e
raise e if e.message !~ /IndexMissingException/
raise e if e.message !~ /IndexMissingException/ && e.message !~ /index_not_found_exception/
Copy link
Contributor

Choose a reason for hiding this comment

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

Which exceptions can be here as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you look at this PR, you will see that IndexMissingException was changed to IndexNotFoundException.

Copy link
Contributor

Choose a reason for hiding this comment

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

Then should not it be || here to support both of versions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's !~ (not match).

For example:
[3] pry(main)> '{"error":{"root_cause":[{"type":"index_not_found_exception","reason":"no such index","resource.type":"index_or_alias","resource.id":"products","index":"products"}],"type":"index_not_found_exception","reason":"no such index","resource.type":"index_or_alias","resource.id":"products","index":"products"},"status":404}'.!~(/IndexMissingException/)
=> true

'{"error":{"root_cause":[{"type":"index_not_found_exception","reason":"no such index","resource.type":"index_or_alias","resource.id":"products","index":"products"}],"type":"index_not_found_exception","reason":"no such index","resource.type":"index_or_alias","resource.id":"products","index":"products"},"status":404}'.!~(/index_not_found_exception/)
=> false

The same for strings which contains IndexMissingException

If we want to reraise exception, when string does not contain IndexMissingException and index_not_found_exception substrings, then we'll use next logic:
reraise exception if message doesn't contain IndexMissingException and index_not_found_exception.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah you are right, sorry, I've mixed something

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 worry, I recheck it few times today 😄 Thanks

@pyromaniac
Copy link
Contributor

So are you considering this PR as ready to be merged?

@sergeygaychuk
Copy link
Contributor Author

I think yes. If I find some new issues with v2.x, then I'll do separate PR.

pyromaniac added a commit that referenced this pull request Dec 17, 2015
Add support for elasticsearch v2.0...v2.1
@pyromaniac pyromaniac merged commit daddac7 into toptal:master Dec 17, 2015
@sergeygaychuk
Copy link
Contributor Author

@pyromaniac But we need to add some tests to Travis CI. Really, I don't how to do it :(

@pyromaniac
Copy link
Contributor

I know :) Will do it eventually

@sergeygaychuk
Copy link
Contributor Author

:) Thanks

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.

2 participants