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

Solr tweaks #1466

Merged
merged 44 commits into from
Jul 13, 2017
Merged

Solr tweaks #1466

merged 44 commits into from
Jul 13, 2017

Conversation

jywarren
Copy link
Member

  • all tests pass -- rake test:all
  • code is in uniquely-named feature branch, and has been rebased on top of latest master (especially if you've been asked to make additional changes)
  • pull request is descriptively named with #number reference back to original issue

Please be sure you've reviewed our contribution guidelines at https://publiclab.org/wiki/contributing-to-public-lab-software

We have a loose schedule of reviewing and pulling in changes every Tuesday and Friday, and publishing changes on Fridays. Please alert developers on plots-dev@googlegroups.com when your request is ready or if you need assistance.

Thanks!

@jywarren
Copy link
Member Author

jywarren commented Jun 15, 2017

Again back to https://github.com/sunspot/sunspot#manually-adjusting-solr-parameters

Based on console testing, it could also work to remove &defType=edismax, but I'm not sure how to do that as easily in rails compared to just assigning a blank string to params[:qf].

@PublicLabBot
Copy link

PublicLabBot commented Jun 15, 2017

1 Warning
⚠️ It looks like you merged from master in this pull request. Please rebase to get rid of the merge commits – you may want to rewind the master branch and rebase instead of merging in from master, which can cause problems when accepting new code!
3 Messages
📖 @jywarren Thank you for your pull request! I’m here to help with some tips and recommendations. Please take a look at the list provided and help us review and accept your contribution!
📖 This pull request doesn’t link to a issue number. Please refer to the issue it fixes (if any) in the body of your PR, in the format: Fixes #123.
📖 It looks like you haven’t marked all the checkboxes. Help us review and accept your suggested changes by going through the steps one by one. If it is still a ‘Work in progresss’, please include ‘[WIP]’ in the title.

Generated by 🚫 Danger

@jywarren
Copy link
Member Author

OK, here i'm stuck -- bc based on my testing on the Solr console on staging, we need to set params[:qf] = "" -- but that seems to break the tests using embedded Solr.

I can try some other variations, like params[:qf] = "''" -- empty quotes -- but maybe we should instead try removing the edismax part...

@jywarren
Copy link
Member Author

Trying .delete(:defType)... odd but who knows

@jywarren
Copy link
Member Author

Should be able to test this with http://staging.laboratoriopublico.org/searches/test/test?q=stk500_recv

@jywarren
Copy link
Member Author

jywarren commented Jun 15, 2017

Unfortunate... @icarito -- any inspirations?

In summary:

We know the exact Solr query we want, we just can't get Sunspot to create it.

Alternatively, we aren't sure why the default Sunspot-generated query (with qf=title_text+body_text+comment_text and defType=edismax) doesn't work -- removing either of those portions of the query gets us full results.

Actually, to be specific, using adjust_solr_params to change qf to nil works properly on the staging server, but it causes the embedded Solr to return no results.

@jywarren
Copy link
Member Author

@jywarren
Copy link
Member Author

Trying to set params[:defType] to an empty string, which on rails console testing resulted in:

irb(main):011:0> s = Node.search { fulltext('balloon');adjust_solr_params { |p| p[:defType] = '' } }.results
  SOLR Request (109.0ms)  [ path=select parameters={fq: ["type:Node"], q: "balloon", fl: "* score", qf: "title_text body_text comments_text", defType: "", start: 0, r
ows: 30} ]

And full results...

@jywarren
Copy link
Member Author

OK, another variation which worked on external Solr but did not pass embedded solr tests:

Failure:
  <[]> expected to be != to
  <[]>.
test_should_get_search_test_action(SearchesControllerTest)
test/solr/searches_controller_test.rb:23:in `block in <class:SearchesControllerTest>'

Sebastian -- the alternative here is that we could simply drop support of the embedded gem sunspot_solr engine (which is for development only anyways) and spin up the containerized Solr in Travis.

What would that take? Changes to:

  • uncomment the solr container in docker-compose.yml (for purposes of Travis only)
  • repoint /config/sunspot.yml to the container (just the port number, i guess)
  • would the solr-specific test.rake lines require tweaks so that the commands to start/stop the solr engine would go to the containerized solr, not the embedded solr?

This seems like a way forward even if we don't figure out what's going on. I suspect the embedded solr is just an older version and is not responding to the same queries in the same ways. Maybe it's not using edismax queries, for example.

@jywarren
Copy link
Member Author

Actually i like this plan a lot. It's closer to how we'll be deploying anyways.

@jywarren
Copy link
Member Author

Attempting these steps now -- not 100% sure of the sunspot.yml or .travis.yml or test.rake changes, but let's see how it does!

@jywarren jywarren force-pushed the solr-no-query-fields branch from 86f9ac1 to b02f033 Compare June 19, 2017 01:16
@jywarren
Copy link
Member Author

Seems close -- solr container starts up, but it seems to run /after/ bower install for some reason, though i tried adding 40 seconds wait. The last thing that runs is WeBrick starting Rails, but i never see rake db:migrate run, or tests.

@icarito icarito force-pushed the solr-no-query-fields branch 5 times, most recently from 301b4a1 to 418233c Compare June 22, 2017 18:14
@jywarren
Copy link
Member Author

Hmm, so what is going on here? Can you push up a copy without logging on, not a force push? What's the specific error?

@icarito icarito force-pushed the solr-no-query-fields branch from 418233c to f6ede88 Compare June 22, 2017 20:49
@icarito
Copy link
Member

icarito commented Jun 22, 2017

Logging is the only place where we are seeing the actual error.
rake db:migrate is run without errors: https://travis-ci.org/publiclab/plots2/builds/245957757#L2999 check the raw log...

@icarito
Copy link
Member

icarito commented Jul 7, 2017

Ok, so the reindex appears to be working fine. I'll try to reproduce in a local environment.

@jywarren
Copy link
Member Author

Hi, Sebastian -- how's this going?

@icarito
Copy link
Member

icarito commented Jul 11, 2017

I don't why, I thought we had solved it. Likely needs a last push, will review it now. Sorry!

@icarito
Copy link
Member

icarito commented Jul 11, 2017

I've merged this into unstable branch so that we can use jenkins to track this.
This made me notice that in our current setup, staging and production are sharing the same pad.publiclab.org Solr index! At one point I stopped the Solr container in pad.publiclab.org and experienced a 500 error in publiclab.org/dashboard! So I think we need to doublecheck our fallbacks.

@jywarren
Copy link
Member Author

jywarren commented Jul 11, 2017 via email

@icarito
Copy link
Member

icarito commented Jul 11, 2017

Yes! I've closed the firewall at pad.publiclab.org to only accept connections from publiclab.org (dropping requests from tycho.media.mit.edu). That should ensure isolation from staging. I'm going to set SOLR_URL in Jenkins, it should override the config in sunspot.yml.

@icarito
Copy link
Member

icarito commented Jul 11, 2017

In fact, having firewalled tycho from pad.publiclab.org should affect staging and allow to debug the fallback. Checking it...

@icarito
Copy link
Member

icarito commented Jul 11, 2017

I'm not seeing the staging server fail after restricting access to pad.publiclab.org. I'll check it further tonight.

@icarito
Copy link
Member

icarito commented Jul 11, 2017

My fault, I had put the wrong address in SOLR_URL env variable in jenkins config. It appears to be working fine after changing it.

@icarito
Copy link
Member

icarito commented Jul 11, 2017

I think staging is working except without access to Solr. I'll have a closer look tonight.

@icarito
Copy link
Member

icarito commented Jul 12, 2017

Okay I was trying to reindex tonight and got this:

icarito@tycho:/srv/plots_testing/plots$ RAILS_ENV=production SOLR_URL=http://solr:8983/solr/ docker-compose exec web rake DISABLE_SOLR_CHECK=1 sunspot:solr:rei
ndex
rake aborted!
Errno::ETIMEDOUT: Connection timed out - connect(2) for "pad.publiclab.org" port 8983
(eval):2:in `post'
Tasks: TOP => sunspot:solr:reindex => sunspot:reindex
(See full trace by running task with --trace)
icarito@tycho:/srv/plots_testing/plots$

This is because the firewall is set to DROP and I guess the Solr-check is timing out instead of immediately failing, taking a long time. I'll make a patch to enable Solr container in docker-compose.yml and also try setting the firewall to REJECT, to see if the Solr-check will work better in that case.

@icarito
Copy link
Member

icarito commented Jul 13, 2017

@jywarren sincerely I've spent a lot of time on this one and I can't tell why it's failing. I would appreciate your help.

@jywarren
Copy link
Member Author

Ok we can look at this tomorrow! Thanks for spending time on this.

My first thought is can we view and reproduce this query directly on solr console on staging.

@jywarren
Copy link
Member Author

@icarito
Copy link
Member

icarito commented Jul 13, 2017

Today Jeff and I worked on this, configuring the staging environment to run from the test RAILS_ENV, and discovered that the indexer was indexing the seed data instead of the test data. We're attempting to reindex after the original tests have been run, to see if it will include the test data.

@icarito
Copy link
Member

icarito commented Jul 13, 2017

YESS!

@jywarren
Copy link
Member Author

HOORAAYYYYYYYY

lets get this on staging fast!

@jywarren
Copy link
Member Author

Ready to merge, right? if so, can you hit merge and i'll push to staging?

@icarito
Copy link
Member

icarito commented Jul 13, 2017 via email

@jywarren jywarren merged commit 840bc36 into master Jul 13, 2017
@icarito
Copy link
Member

icarito commented Jul 13, 2017

Note that staging will be broken until I restore the staging database from a dump. I can do it later today.

@icarito
Copy link
Member

icarito commented Jul 14, 2017

Staging is back with a reindexed solr container, all lights green.

@emilyashley emilyashley deleted the solr-no-query-fields branch January 15, 2020 21:35
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.

3 participants