Skip to content

Rip sphinx 755923#666

Closed
willkg wants to merge 7 commits into
mozilla:masterfrom
willkg:rip-sphinx-755923
Closed

Rip sphinx 755923#666
willkg wants to merge 7 commits into
mozilla:masterfrom
willkg:rip-sphinx-755923

Conversation

@willkg
Copy link
Copy Markdown
Member

@willkg willkg commented Jun 20, 2012

I think this:

  1. removes all the Sphinx related stuff
  2. nixes the elasticsearch waffle flag
  3. ports all the Sphinx tests to ES unified test case (making it comprehensive)
  4. fixes search suggestions

What I did to test it was:

  1. run the tests

I'm not sure what else helps test this. I think the issues are modules not loading because specific imports aren't available anymore and other things failing because they expected Sphinx and we didn't update it for ES, but Sphinx is not around anymore.

Anyhow, r?

willkg added 4 commits June 20, 2012 14:10
This removes all the Sphinx-related stuff.

You may need to do:

    git rm --cached vendors/src/oedipus vendor/src/sphinxapi

to remove the vendor git submodule stuff.
This rewrites it to use ES .search() code rather than whatever it was
using before. Also, the index layout has changed, so this updates
that, too.
If a search adds filters on model Foo, but not on other models, then
what happens is that the Fs get combined and the resulting F requires
things that are only in model Foo and thus the search results only
have results from model Foo in them.

This fixes that by forcing the F to specify which model they filter on.
So when they get combined, we get filters like "if the model is Foo AND
this AND that" OR "if the model is Bar AND this AND that" ... which
alleviates the combination problem.
This ports all the Sphinx tests we didn't have implemented for ES
to ES.
@rlr
Copy link
Copy Markdown
Contributor

rlr commented Jun 20, 2012

"573 additions and 2,135 deletions. " :-)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I beat you to landing a 148 migration.

@willkg
Copy link
Copy Markdown
Member Author

willkg commented Jun 20, 2012

I remembered that there were three tests I pulled in from test_sphinx.py that were automatically skipped. I fixed two of them. In the process of fixing the third, I found a minor bug with forum filtering.

I'm finishing up those fixes now and will add them to this pull request in a jiffy.

willkg added 3 commits June 20, 2012 17:14
This fixes the migration for the elasticsearch waffle removal.
* forum has a u. oops.
* if we hit ES and it's all like, "d00d! there are no things!", then
  we know there are no things and we don't have to ask it a second
  time to return an empty list.
These tests were in test_sphinx, but were automatically skipped. This
fixes them for ES and fixes the test_discussion_filter_forum test
which had a subtle issue.
@willkg
Copy link
Copy Markdown
Member Author

willkg commented Jun 20, 2012

I added three more commits. They should fix the outstanding problem plus fixes bunch of tests that were automatically skipped that I copied over from test_sphinx, but forgot to fix. Also, fixed a bug with filtering on forum ids.

@rlr
Copy link
Copy Markdown
Contributor

rlr commented Jun 20, 2012

20 secs off my local test suite run \o/

@rlr
Copy link
Copy Markdown
Contributor

rlr commented Jun 21, 2012

r+. When is the funeral?!

@willkg
Copy link
Copy Markdown
Member Author

willkg commented Jun 21, 2012

LANDED!

8405999 and friends

@willkg willkg closed this Jun 21, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants