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

Added 'query_cache' option for search #886

Merged
merged 2 commits into from
Jul 7, 2015
Merged

Conversation

JanJakes
Copy link
Contributor

@JanJakes JanJakes commented Jul 7, 2015

No description provided.

@@ -22,6 +22,7 @@ class Search
const OPTION_SIZE = 'size';
const OPTION_SCROLL = 'scroll';
const OPTION_SCROLL_ID = 'scroll_id';
const OPTION_QUERY_CACHE = 'query_cache';
Copy link
Owner

Choose a reason for hiding this comment

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

To make this work, it must also be added to _validateOptions: https://github.com/ruflin/Elastica/blob/master/lib/Elastica/Search.php#L273 Best would be to add a small tests to make sure it works as expected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, fixed. I'll have a look how to run the tests but if it takes more time I'm not sure when I can get to that.

Copy link
Owner

Choose a reason for hiding this comment

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

In case you have boot2docker running, it is quite simple:

make run RUN="phpunit -c ./test lib/Elastica/Test/SearchTest.php"

This runs the SearchTest.php file. The first time will take a little bit longer as it must fetch all the docker images.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I've added the test. Btw. the query_cache option will probably work only with search_type=count but that's expected behavior (https://www.elastic.co/guide/en/elasticsearch/reference/1.6/index-modules-shard-query-cache.html#index-modules-shard-query-cache).

ruflin added a commit that referenced this pull request Jul 7, 2015
Added 'query_cache' option for search
@ruflin ruflin merged commit 882e89f into ruflin:master Jul 7, 2015
@ruflin
Copy link
Owner

ruflin commented Jul 7, 2015

Merged, thanks. I would be not surprised if soonish query_cache is also supported for other search_types the way the doc is written. Lets see :-)

@ruflin
Copy link
Owner

ruflin commented Jul 7, 2015

@JanJakes Just for my own feedback: Was there any issue in setting up the project to run the tests?

@JanJakes
Copy link
Contributor Author

JanJakes commented Jul 7, 2015

@ruflin It was actually supereasy :) Just running boot2docker and the single make run command exactly like you said. At first I just didn't notice that I need absolutely no setup to run these commands - https://github.com/ruflin/Elastica/blob/master/CONTRIBUTING.md#run-tests.

@ruflin
Copy link
Owner

ruflin commented Jul 8, 2015

@JanJakes Thanks for the feedback. No setup (except boot2docker) is the beauty of it ;-) You mean I should emphasize that there is REALLY no other setup needed?

@JanJakes
Copy link
Contributor Author

JanJakes commented Jul 9, 2015

@ruflin Yeah, maybe :) I mean it's not a standard (yet) to have repos prepared with Docker bootstrap as simple as this one.

@ruflin
Copy link
Owner

ruflin commented Jul 10, 2015

Not yet. Probably the CONTIRUBTING pages needs a tl;dr

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.

2 participants