Skip to content

Prevent double calls to search.execute! when also querying shards, aggregations, and suggestions' #847

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

Closed
wants to merge 1 commit into from

Conversation

coneybeare
Copy link

I was noticing a ton of duplicate search logs in my app, and tracked it down to the Response.response method running a search.execute! in which the raw_response is not being saved, causing subsequent calls to query shards, aggregations, and suggestions to perform another search.execute!. call An example of when this might occur is the following

# Setup search with query and aggregations
response = Post.search('foo')
# Actually execute the search and get results
results = response.results
# Make a second execution because response's `raw_response` is not being cached
aggregations = response.aggregations

By simply storing the search.execute! return in the @raw_response var, we can utilize the cached raw_response in the subsequent calls and prevent double searches.

I have submitted 2 pull requests prior to this one that ended up having pitfalls so I closed them. After some more digging in, I think this simple approach works well. It passes all tests on master, doesn't interfere with pagination (like previous attempt 1) and doesn't add any additional configuration or parameters (like previous attempt 2). Please consider a merge.

@karmi
Copy link
Contributor

karmi commented Oct 8, 2018

Hi @coneybeare, thanks for catching this! We'll definitely look into this — re-using the @raw_response is definitely a good approach, we'll have to check if there are not some thread-safety etc issues.

@estolfo
Copy link
Contributor

estolfo commented Oct 15, 2018

Hi @coneybeare
I've opened a pull request as well to solve this here

The reason we have the raw_response and response methods is because using HashWrapper can sometimes have large performance implications (see this issue). The raw_response method allows someone to skip creating a HashWrapper and just get the raw response in the form of a Hash. I've taken a slightly different approach to you and just rewritten the #raw_response method so that if:

  1. A HashWrapper has already been created from a search response, the Hash representation of it is returned.
  2. A HashWrapper hasn't been created yet from a search response, it executes the search and returns the raw Hash response.

What do you think?

@estolfo
Copy link
Contributor

estolfo commented Oct 16, 2018

Closing in favor of #850

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