Skip to content
This repository was archived by the owner on Aug 26, 2022. It is now read-only.

Es search view 839214#923

Merged
darkwing merged 9 commits into
mdn:masterfrom
groovecoder:es-search-view-839214
Mar 19, 2013
Merged

Es search view 839214#923
darkwing merged 9 commits into
mdn:masterfrom
groovecoder:es-search-view-839214

Conversation

@groovecoder
Copy link
Copy Markdown
Contributor

(Would like to get @lmorchard and/or @ubernostrum to think about my forcing the override of settings from test-utils)

Supersedes #898. The same testing instructions apply, plus some new ones:

You'll need to fetch my kuma-lib repos, which means the leeroy/mdn-github jenkins build is going to fail.

  1. git submodule update --init --recursive
  2. vagrant provision
  3. Run tests. Make sure to include the 'search' app.

To spot-check, make sure you're running ./manage.py celeryd, then:

  1. update a doc in the site UI
  2. go to https://developer-local.allizom.org/elastic/_plugin/head/
  3. connect it to https://developer-local.allizom.org/elastic/
  4. go to the 'Browser' tab and make sure there's a record in the mdn-main_index for the doc

To spot-check the search view (wiring only, nothing fancy):

  1. go to https://developer-local.allizom.org/admin/waffle/flag/
  2. add flag 'elasticsearch' active for your user
  3. go to https://developer-local.allizom.org/en-US/search

You should see a simple search result list matching the ES index.

elasticutils tasks broken; add tasks in search app

test
Code borrowed in large part from mozilla/fjord

update elasticutils, using pyelasticsearch with updated requests library

update test-utils library to eagerly test celery tasks
don't ignore settings_test
remove sphinx modules from search app
@lmorchard
Copy link
Copy Markdown
Contributor

Think I have time to peek at this right now...

@openjck
Copy link
Copy Markdown
Contributor

openjck commented Mar 13, 2013

Is this behind a feature flag, or will the view be entirely powered by Elastic Search once this lands? Just want to be sure we have high quality results before this becomes official, considering how important search is to the site.

@groovecoder
Copy link
Copy Markdown
Contributor Author

Yup, there's an 'elasticsearch' feature flag on the view

@groovecoder
Copy link
Copy Markdown
Contributor Author

There's also an ES_DISABLED setting that is set to True by default, which prevents any of the backend operations. We won't enable it in production until everything is configured; it looks like MDN already has flows to RabbitMQ on dev and to ES on dev, stage, and prod.

@lmorchard
Copy link
Copy Markdown
Contributor

Attempt #2 at finding time to look at this. First thing, the "my forcing the override of settings from test-utils" link in the description leads to an attempt to create a new pull request (ie. pull/new). Not sure to what this was meant to link, but might be able to figure it out.

@lmorchard
Copy link
Copy Markdown
Contributor

Okay, found the link for "my forcing the override of settings from test-utils", but not quite sure what's happening there. @ubernostrum might have something smarter to say about it, since I just basically stole the code from a django snippet and I'm not quite sure what UserSettingsHolder does.

@lmorchard
Copy link
Copy Markdown
Contributor

Tests seem to pass ok:

vagrant@developer-local:/vagrant$ (cd /; /vagrant/manage.py test search wiki actioncounters contentflagging demos devmo landing users)
...
Ran 491 tests in 164.760s

OK (SKIP=47)

Moving on to spot checking after getting some lunch...

@lmorchard
Copy link
Copy Markdown
Contributor

Working through spotcheck, getting an exception in kumascript execution that seems related to the Requests module upgrade in kuma-lib. Investigating how this might be happening.

@lmorchard
Copy link
Copy Markdown
Contributor

Yeah, looks like the Requests API has changed significantly from v0.6.1 (what MDN uses now) to v1.1.0 (what your kuma-lib uses), and that breaks in kumascript.py.

The fix is probably just one line, but I'm not sure what else might break with the Requests upgrade - especially since we try to avoid doing real HTTP requests in tests that would use Requests.

@groovecoder
Copy link
Copy Markdown
Contributor Author

Hmm ... that sounds familiar. I want to to say I fixed it already and maybe I have a kumascript commit to push for it?

@groovecoder
Copy link
Copy Markdown
Contributor Author

Updated kumascript tests to match response api from new version of requests library. All tests passed for me!

@lmorchard
Copy link
Copy Markdown
Contributor

So, tests all pass now, but:

  • tests are still hammering elasticsearch with real requests like PUT /mdn-main_index/wiki_document/447 HTTP/1.1, and
  • those tests are using "mdn-main_index"

Are these something that the test settings should be overriding?

@groovecoder
Copy link
Copy Markdown
Contributor Author

ARGH! Forcing into settings.__dict__ is not so easy to disable. So any tests that run after the override are back to hitting the real index. :( Been banging on it for an hour or so now ... any ideas?

@groovecoder
Copy link
Copy Markdown
Contributor Author

OMG finally fixed with help from @willkg ...

Ran 520 tests in 101.900s

OK (SKIP=66)

And only the task test hits the ES index.

@lmorchard
Copy link
Copy Markdown
Contributor

none of the tests should hit a real es service though. there's no es on Jenkins and in general we need to mock out network services

@willkg
Copy link
Copy Markdown
Contributor

willkg commented Mar 18, 2013

Just as a thing, Jenkins (ci.mozilla.org) does have an ES. We use this for kitsune, fjord, amo, etc. We sort of think of this like we think of the db.

@lmorchard
Copy link
Copy Markdown
Contributor

ah cool. then ignore my grumblings and I'll plan on giving this another look when I'm not on airport wifi

@darkwing
Copy link
Copy Markdown
Contributor

Sport-check worked, tests passed for me.

darkwing added a commit that referenced this pull request Mar 19, 2013
@darkwing darkwing merged commit a85a724 into mdn:master Mar 19, 2013
@lmorchard
Copy link
Copy Markdown
Contributor

Ack! This really needed to be squashed!

@lmorchard
Copy link
Copy Markdown
Contributor

Also, did the kuma-lib change get merged too? Need to do that fast before things break

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants