-
-
Notifications
You must be signed in to change notification settings - Fork 86
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
Support Elasticsearch 7 #263
Conversation
- Remove the doc_type. - Move the definition to the root of the document - Reformat file
Code Climate has analyzed commit d1057c5 and detected 1 issue on this pull request. Here's the issue category breakdown:
View more on Code Climate. |
Hi m4rcu5, thanks for your PR. This is something I've been wanting to do because I run ES 7, but I haven't gotten a chance to make the change. I'll take a look at this in the next few days and get back to you with any feedback as soon as I can. |
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.
Overall changes look reasonable. Only two minor clarity changes, the others are notes for the natlas project in the future.
@@ -83,7 +97,7 @@ def collate_source(self, documents): | |||
def execute_search(self, **kwargs): | |||
''' Execute an arbitrary search.''' | |||
with self._new_trace_span(operation='search', **kwargs) as span: | |||
results = self._execute_raw_query(self.es.search, doc_type='_doc', **kwargs) | |||
results = self._execute_raw_query(self.es.search, doc_type='_doc', rest_total_hits_as_int=True, **kwargs) |
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.
This flag will be deprecated in ES 8.x. It's okay to add this for now so we can provide natlas users with a time period when they can use 6.x and 7.x, but we'll have to switch to the new version eventually.
@0xdade Let's put a milestone on the natlas roadmap where we'll break ES 6.x compatibility and switch to ES 7.x as a minimum. We should put a release note in this next Natlas version as an FYI that will be deprecated.
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.
When dropping any pre-7 support, you could rewrite the code that looks for a count to check the value
attribute. As these are quite some places, this looked like the cleanest way to keep backwards compatibility, without rewriting all those snippets to check if its a dict or int.
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'll make the most sense to drop ES 6.x support when we upgrade to natlas 0.7.x, which will indicate a backwards incompatible change. I haven't really roadmapped out the features for this yet, though.
@ajacques thanks for the review. I've ran the docker containers against Elasticsearch 7.6.2 and Elasticsearch 6.8.2, both worked as expected. |
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.
Everything looks good. Thanks for your contribution.
To summarize, we'll maintain ES 6.x compatibility through natlas 0.6.x, then deprecate at 0.7.0
This PR creates support for Elasticsearch 7 while preserving 6.x compatibility. It could close #234