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

Add batch search endpoint #1299

Merged
merged 2 commits into from
Nov 1, 2018
Merged

Add batch search endpoint #1299

merged 2 commits into from
Nov 1, 2018

Conversation

oscarwyatt
Copy link
Contributor

@oscarwyatt oscarwyatt commented Oct 18, 2018

Trello: https://trello.com/c/3KbtCe3D/123-add-batchsearch-endpoint-to-rummager

Adds a batch search endpoint so that multiple searches can be processed with a single Rummager query. Uses Elasticsearch multi query to process the searches efficiently as a single Elasticsearch call.

It can process multiple searches with an example url such as:

/batch_search?search[][0][count]=10&search[][0][q]=ministry+of+magic&search[][0][order]=-popularity&search[][1][start]=0&search[][1][q]=floo+network+bandwith+enquries

It allows a maximum of 10 searches per request at the moment. There is no technical reason for this it's completely arbitrary so this can be changed in the future if the need arises

@vanitabarrett vanitabarrett changed the title [WIP][Do not merge]Add batch search endpoint Add batch search endpoint Oct 29, 2018
@vanitabarrett vanitabarrett force-pushed the add-batch-search branch 3 times, most recently from e632103 to 88d883c Compare October 29, 2018 10:38
sihugh
sihugh previously requested changes Oct 31, 2018
Copy link
Contributor

@sihugh sihugh left a comment

Choose a reason for hiding this comment

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

This looks pretty good. I've a few comments about style/lint type things, just so we can keep the git history clean.

class BatchQuery < Query
class TooManyQueries < Error; end
def run(searches_params)
raise(TooManyQueries, 'Maximum of 10 searches per batch') unless searches_params.count <= 10
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add something to the commit message to indicate that this is a fairly arbitrary number and it's small because we're cautious?

end

def log_search_count(searches)
GovukStatsd.increment "batch_search_#{searches.count}_searches"
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice 👍

@@ -61,39 +90,13 @@ def run_spell_checks(search_params)
return unless suggestion_blocklist.should_correct?(search_params.query)

query = {
size: 0,
suggest: QueryComponents::Suggest.new(search_params).payload
size: 0,
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure why the linter didn't pick up extra whitespace 🤔 It'd be good to remove this, as it will preserve the blame trail better.

process_es_response(search_params, builder, payload, es_response)
end

private
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think very much has changed here. I wonder if it's possible to split the method extraction into a separate commit. It might make it easier to reason about when we come back to it in future?

"format" => "local_transaction",
"link" => "/URL",
"_type" => "edition",
"title" => "TITLE1",
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment about ditching the whitespace as before - if nothing's changed we don't want to obscure the git history.

@vanitabarrett
Copy link
Contributor

vanitabarrett commented Nov 1, 2018

@oscarwyatt Would be good to get the linting errors fixed so we unblock merging of alphagov/gds-api-adapters#855

@oscarwyatt
Copy link
Contributor Author

oscarwyatt commented Nov 1, 2018

@oscarwyatt Would be good to get the linting errors fixed so we unblock merging of alphagov/gds-api-adapters#855

@vanitabarrett Apologies, fixed now

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

3 participants