-
Notifications
You must be signed in to change notification settings - Fork 368
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
Conversation
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) } |
There was a problem hiding this comment.
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.
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/ |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
So are you considering this PR as ready to be merged? |
I think yes. If I find some new issues with v2.x, then I'll do separate PR. |
Add support for elasticsearch v2.0...v2.1
@pyromaniac But we need to add some tests to Travis CI. Really, I don't how to do it :( |
I know :) Will do it eventually |
:) Thanks |
Tested on:
a) with delete-by-query plugin
b) without delete-by-query plugin
Maybe someone can setup v2.x version on Travis CI.