-
Notifications
You must be signed in to change notification settings - Fork 417
Upgrade of Elastic Search Client#571
Upgrade of Elastic Search Client #571vineetvermait wants to merge 1 commit intomaster from unknown repository
Conversation
vineetvermait
commented
Jun 1, 2017
Issue filed in JIRA: USERGRID-1344
snoopdave
commented
Jun 3, 2017
Thanks for the PR.
There is a large amount of formatting changes in the PR that makes it difficult to see what has changed. Please submit a clean PR that contains only the changes you wish to make and makes no formatting changes. Also, new code should follow the existing code style of Usergrid which uses spaces for indentation and no tabs.
@snoopdave , As requested, removed the code formatting. Added just the updated code
snoopdave
commented
Jun 17, 2017
Thanks Vineet, this PR looks much better. A better description of this PR would be "Update the Chop testing framework from ElasticSearch 1.4.x/1.7.x to ElasticSearch 5.4.x" because this PR does not upgrade Usergrid itself to the new ElasticSearch -- just the Chop testing framework.
(I wonder how much work it would be to upgrade Usegrid itself to ES 5.4.x)
I think we should accept this PR. Any thoughts @michaelarusso and @mikedunker-apigee?
michaelarusso
commented
Jun 18, 2017
I'm not as familiar with the Chop testing framework. However, I can't see how it's beneficial to add 5.4.x Elasticsearch dependencies to the testing framework when Usergrid itself is not updated to support anything newer than 1.x of Elasticsearch.
In terms of effort to add support for later versions of Elasticsearch, I imagine this to be a big effort. There will be breaking changes, maybe many, between 1.x and 5.x versions of Elasticsearch. The approach has to be thought through very carefully, otherwise existing users of Usergrid will be left in an awkward situation when trying to upgrade. New users might be fine, but the breaking changes in the cluster for existing users may just support a major version bump in Usergrid itself.
snoopdave
commented
Jun 18, 2017
Since the Usergrid project does not use Chop, changes to Chop don't impact Usergrid -- Chop is a separate project that probably shouldn't even be part of the Usergrid codebase since it results in confusion.
If Vineet is using Chop separately from Usergrid, perhaps these changes make sense to him -- and maybe we should accept them.
If the intention of this PR was to upgrade Usergrid to ES 5.x, then that is not what it does -- and we should not accept the changes.
michaelarusso
commented
Jun 18, 2017
@snoopdave Thanks for the clarification. Given that Chop is separate, do you think it makes sense to create another repo, usergrid-chop, and move the code over post-merge?
vineetvermait
commented
Jun 21, 2017
@snoopdave @michaelarusso I would like to know if chop is the only component which uses Elastic Search and if elastic search is even one of the core components of User-grid or not?
I had made the changes because while installing User-Grid, I used the latest Elastic Search version and got client issues in the logs. I updated the Client and tested and it works perfectly fine now.
Therefore for User Grid to work with latest version of Elastic Search the changes are needed.
Arranging Chop in a separate repository is another call.
Please suggest
vineetvermait
commented
Jun 23, 2017
Hi @snoopdave, is the pull request being taken up..?
snoopdave
commented
Jun 23, 2017
This pull request does not upgrade Usergrid to use ES 5.4.x, it only upgrades the completely separate Chop testing framework that is not part of Usergrid and is not used by Usergrid -- so I don't think we need it.
If you want to upgrade Usergrid, you need to change the stack/corepersistence/queryindex module, that is how Usergrid interfaces with ES.
vineetvermait
commented
Jun 27, 2017
Hi @snoopdave,
Would like to confirm, if we can deploy User grid current version with Elastic Search 5.4.x and run smoothly.
Please confirm
snoopdave
commented
Jun 27, 2017
via email
vineetvermait
commented
Jun 30, 2017
Hi @snoopdave i tried that and faced a lot of issues...
the system showed compatibility issues in logs mentioning in API response about version incompatibility.
Thus my reason to create this pull request
vineetvermait
commented
Jul 4, 2017
Hi @snoopdave any update?
vineetvermait
commented
Jul 18, 2017
Hi @snoopdave, Please confirm if the PR is going to be merged..??
snoopdave
commented
Jul 18, 2017
I do not intended to merge this code. I don't think it makes sense to upgrade the Chop testing framework to ES 5 when Usergrid itself only supports ES 1.
e2333ad to
893a04d
Compare
1bf227b to
e12fbcb
Compare
Upgrade from:
To