Skip to content
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

733102 es and db #549

Closed
wants to merge 2 commits into from
Closed

733102 es and db #549

wants to merge 2 commits into from

Conversation

willkg
Copy link
Member

@willkg willkg commented Mar 28, 2012

Oo--I'm using the newly placed "pull request" button that reduces my need to select the branch from the list. I kind of like it.

Anyhow, this fixes some tests, then reworks the search_view_es so it doesn't touch the db at all. Good explanation of what changed in the commit comments.

Things to pay attention to for a peer review:

  • I had to rename some things in the index. I'm pretty sure I got the renaming correct, but it's worth double-checking num_votes vs. num_votes_past_week.

Mmm... actually that's it. The rest is pretty straight forward.

I think this will have some improvement to performance, but since db activity is cached and it's pretty small already for the search view, I don't think it'll be noticeable like the previous performance changes were.

r?

* this corrects the queries so the tests test exactly what they're
  meant to test.
* changed the names of some of the fields indexed to correspond with
  the model names they derive from
* add num_votes to question document index
* carry question document index changes through codebase
* change answer_vote model maker to answervote
* remove excerpting test that's no longer pertinent
* remove group_by call which we don't support with ES
* add code so when QuestionVote is saved, it updates the index
* simplify the pagination code
* tweak some TODO and other comments
* some other cleanup

End result is that a search with ES no longer hits the DB at all
but instead pulls everything from the index. Additionally, simplified
a bunch of silly stuff out of the ES search view.

Note: This requires reindexing.
@@ -584,6 +601,10 @@ def add_metadata(self, key, value):
VoteMetadata.objects.create(vote=self, key=key, value=value)


register_for_indexing(
Copy link
Contributor

Choose a reason for hiding this comment

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

This is cool. I probably saw this a few times before but now I finally realized how cool it was :-)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah... so this is something Erik did and while it's kind of neat, I kind of think it does too much and does some of it in magic ways. But I haven't thought of a better way that's as succinct, but more legible and less magic.

@rlr
Copy link
Contributor

rlr commented Mar 28, 2012

Nice cleanup, tests pass, looks good r+

@willkg
Copy link
Member Author

willkg commented Mar 29, 2012

landed in master: ab4919f

@willkg willkg closed this Mar 29, 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.

None yet

2 participants