mozilla / elasticutils

[deprecated] A friendly chainable ElasticSearch interface for python
http://elasticutils.rtfd.org
BSD 3-Clause "New" or "Revised" License
243 stars 76 forks source link

Migrated to elasticsearch-py, the official ES client #176

Closed honzakral closed 10 years ago

honzakral commented 10 years ago

this should provide more compatibility going forwards as well as performance boost.

Currently I only migrated code and tests, docs are still WIP. Any feedback is more than welcome.

robhudson commented 10 years ago

Overall this looks good to me.

Since I just used the official elasticsearch client during the readthedocs hackathon this weekend and wanted to use elasticutils for the search queries (but chose not to in the short term to avoid 2 elasticsearch libs), this is exciting to me. But, I know I also use this project on a much larger code base at Mozilla and it would be a blocker to upgrading in the short-term because we'd have to upgrade everywhere else we use pyelasticsearch.

I'm wondering:

  1. Do the other maintainers think this is the way to go. I, for one, feel like it is.
  2. Assuming (1) is a yes, how best to roll this out to give people time to upgrade. Perhaps 2 versions for awhile -- one (the older and getting back ported patches) using pyelasticsearch while the newer version going forward uses the official client? Perhaps 0.9 is the pyelasticsearch one and only gets bugfixes on minor releases, whereas 1.0 is the way forward?
willkg commented 10 years ago

I haven't had a chance to look at the code changes, yet. However, there are Travis failures that need looking at and this doesn't update any of the documentation in docs/.

Speaking to @robhudson's thoughts, I know switching from pyes to pyelasticsearch orphaned Mozillians. I don't know if they've updated or not by now. I didn't talk to anyone else who was dead set on using pyes and thus stayed on an older version of ElasticUtils.

Regardless, it's not hard to just have a bunch of branches around and fix bugs in older branches if we/I need to.

Ergo, I vote we switch to elasticutils-py for 0.9.

I'll try to find some time this week to go through this after someone looks into the Travis errors. Then once it lands, I can test it on Input and SUMO and we can go from there. We'll definitely need to update the documentation in docs/ before this gets released.

honzakral commented 10 years ago

I believe that the travis failure is a false positive - locally everything works just fine for me and when I enabled travis for my fork and the build passed there as well: https://travis-ci.org/HonzaKral/elasticutils/builds/13241045 for the exact same commit

I am more than willing to work on the docs and provide a patch there, I could get around to it next week probably. I just wanted to get the patch out there for people to look at and to know if there is demand.

robhudson commented 10 years ago

I hooked this up to the readthedocs codebase (which is already using the official elasticsearch python client) and it worked just fine. I ran into a couple other issues with Elasticutils itself vs the raw queries we're currently doing and filed but that doesn't affect this patch.

After reviewing and testing against a project I'm :+1: on this getting merged (after docs are updated).

honzakral commented 10 years ago

I updated the docs, please let me know if there is anything else I can do to help.

willkg commented 10 years ago

I haven't had time to look at this again. I want to do two things:

  1. read through the docs so I know what's going on
  2. test it out with some samples just to make sure it's all good

I'll try to get that done this week so we can land this soon.

honzakral commented 10 years ago

@willkg thanks! If there is anything I can help with ping me here or on irc (honzakral in #elasticsearch@freenode).

willkg commented 10 years ago

This looks fine to me. I'll merge it and then add a big note in the CHANGELOG.

Thank you!

willkg commented 10 years ago

Landed in:

Yay!