search

MrDys / blacklight

Blacklight Plugin
http://projectblacklight.org/
Other
1 stars 1 forks source link

Factor out use of RSolr's pagination feature and use Solr's start/rows parameters instead #550

Closed MrDys closed 12 years ago

MrDys commented 12 years ago

CODEBASE-379: Factor out use of RSolr's pagination feature and use Solr's start/rows parameters instead.

  1. We decided as a general standard to try not to use any RSolr magic keys, and just pass Solr values directly.
  2. More pressing, current code trying to use RSolr's magic pagination keys is running into weird bugs involving string/symbol key conflicts. (Currently mistakenly filed in rsolr-ext, even though it's really an rsolr issue, we think. https://github.com/mwmitchell/rsolr/pull/31) Not entirely clear how to fix it, we should spend our time on removing BL's use of RSolr magic pagination keys instead.

It is possible that even after we've removed use of RSolr magic pagination keys, we're still going to have trouble with pagination and RSolr, it needs to be tested, and if still issues debugged and RSolr patch request sent.

Erik currently assigned, but Naomi suggested jkeck might have time/interest to take this over.

MrDys commented 12 years ago

Original reporter: jrochkind

MrDys commented 12 years ago

jrochkind: dfunk, feel free to assign to yourself and see what you can do with it. Current BL master has a dependency locked to an old version of RSolr I think, you'd want to unlock that and test with most recent after refactoring, since part of the point is getting it to work with recent RSolr.

MrDys commented 12 years ago

jrochkind: Previous BL generators generated a blacklight_config using :per_page in default_solr_params

I will change the generator to use :rows, AND I will make the SolrHelper#add_paging_to_solr accept this old deprecated behavior (translate it to :rows before sending it to Solr/RSolr).

But release/upgrade notes should probably suggest people change their blacklight_config to use :rows instead of :per_page.

I am not sure whether I should have BL output a deprecation notice to logs if you are using :per_page. It'd be every time SolrHelper#something is called, not just at startup, to catch all possibilities. Any opinion?

MrDys commented 12 years ago

jrochkind: commited as 188b4f1db2

It is converting :per_page in solr_default_params to :rows before sending to Rsolr/Solr, because of legacy generator that generated a blacklight_config with :per_page in solr_default_params. But it probably won't recognize this legacy key forever.

It's not currently reporting deprecation to log for :per_page in solr_default_params, Leave this open to see if anyone wants it to.

MrDys commented 12 years ago

jrochkind: HOWEVER. I am afraid that even with this change, upgrading to latest RSolr still causes tests to fail. For me anyway. I lack time to try to diagnose this.

MrDys commented 12 years ago

cbeer: Which tests are failing for you?

MrDys commented 12 years ago

jrochkind: deprecation warning. Wasn't sure how best to do it, it's just written to $stderr now, and hints that you might want to check default_solr_params. (By the time we catch deprecated behavior, cant' be sure what component may have used :per_page)