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

Count api to become a shortcut to the search api #11198

Merged

Conversation

javanna
Copy link
Member

@javanna javanna commented May 16, 2015

The count api used to have its own execution path, although it would do the same (up to bugs!) as the search api. This commit makes it a shortcut to the search api with size set to 0. The change is made in a backwards compatible manner, by leaving all of the java api code around too, given that you may not want to get back a whole SearchResponse when asking only for number of hits matching a query, also cause migrating from countResponse.getCount() to searchResponse.getHits().totalHits() doesn't look great from a user perspective. We can always decide to drop more code around the count api if we want to break backwards compatibility on the java api, making it a shortcut on the rest layer only.

The only breaking behaviour is around total failures, the search api returns an exception, while the count api used to return a normal response with count set to 0, successful set to 0 and a list of shard failures. Maybe we can find a way to maintain this behaviour, but I'm not sure it makes sense.

If it wasn't for the above breaking aspect we could potentially backport this change to 1.x, for what it's worth. Let's discuss that.

Closes #9117
Closes #9110

@kimchy
Copy link
Member

kimchy commented May 16, 2015

@javanna this looks great!, can we add a test that makes sure that CountRequest converts itself to the right SearchRequest? other than that, LGTM

@javanna
Copy link
Member Author

javanna commented May 16, 2015

Sure @kimchy will add a test. This is the first time (I think) that we make an api shortcut to one another in the java api. The way I did it is very specific to the count api, I wonder if we should support this in a more generic fashion e.g. having some kind of adapter mechanism built-in in AbstractClient rather than having to override three methods to do pretty much the same thing (redirect count to search). Thoughts?

Also, do we see the count api sticking around in the java api or will we remove it at some point? I am conflicted on this, I can't make up my mind.

Last thing, is the breaking aspect around total failures acceptable or is it going to be a problem?

@kimchy
Copy link
Member

kimchy commented May 16, 2015

I wonder if we should support this in a more generic fashion e.g. having some kind of adapter mechanism built-in in

I think that we can add it if we see it becoming a pattern, I doubt it will though.

Also, do we see the count api sticking around in the java api or will we remove it at some point? I am conflicted on this, I can't make up my mind.

Yea, its a tricky one, I think that this change is great without bothering with the question if we should remove it or not, we can decide later.

Last thing, is the breaking aspect around total failures acceptable or is it going to be a problem?

No, I think the consistency here between search and count is better.

@jpountz
Copy link
Contributor

jpountz commented May 17, 2015

Nice diff stats :)

@s1monw
Copy link
Contributor

s1monw commented May 17, 2015

why are we going through the trouble of having a dedicated Java API here? Java folks still need to recompile their code if they wanna use 2.0 so we can just remove it. REST is a different story but has a way lower footprint.

@javanna
Copy link
Member Author

javanna commented May 18, 2015

why are we going through the trouble of having a dedicated Java API here?

I initially wanted to remove the java api layer too, till I realized how much we use the count api in our tests, and I found myself making a change that I didn't like too much: moving to search, adding setSize(0) and replacing getCount() with getHits.getTotalHits(). It seemed easy enough for us to keep request and response around. It's mainly a matter of usability, using the search api directly it's so easy to forget setting size to 0 (and everything still works but much more happens under the hood), count does it internally and allows to set only what is supported when counting only. Also it's nicer to retrieve the resulting count through response.getCount() rather than response.getHits().totalHits(). I personally think it's also nice for java api users to have a corresponding java api for count, rather than having to dig what they have to use instead.

@javanna
Copy link
Member Author

javanna commented May 18, 2015

btw pushed another commit as requested by @kimchy

@javanna
Copy link
Member Author

javanna commented May 20, 2015

I would like to get this in, what do we do? Do we want to keep bwc on the java api layer or not?

@clintongormley
Copy link

It's just sugar, but I think that frequency of use and conciseness is a good enough argument for keeping the count API.

@javanna
Copy link
Member Author

javanna commented May 26, 2015

I agree with @clintongormley I will get this in , @jpountz can you have a last look please?

@jpountz
Copy link
Contributor

jpountz commented May 26, 2015

LGTM

@javanna javanna removed the review label May 26, 2015
The count api used to have its own execution path, although it would do the same (up to bugs!) of the search api. This commit makes it a shortcut to the search api with size set to 0. The change is made in a backwards compatible manner, by leaving all of the java api code around too, given that you may not want to get back a whole SearchResponse when asking only for number of hits matching a query, also cause migrating from countResponse.getCount() to searchResponse.getHits().totalHits() doesn't look great from a user perspective. We can always decide to drop more code around the count api if we want to break backwards compatibility on the java api, making it a shortcut on the rest layer only.

Closes elastic#9117
Closes elastic#11198
@javanna javanna force-pushed the enhancement/count_api_shortcut_search branch from 5b17d22 to 6c81a8d Compare May 26, 2015 17:12
@javanna javanna merged commit 6c81a8d into elastic:master May 26, 2015
s1monw added a commit that referenced this pull request May 26, 2015
The count request now acts like search and barfs if all shards fail
this behavior changed and some tests like RecoveryWhileUnderLoadTests
relied on the lenient behavior of the old count API. This might be
a temporary solution to stop current test failures.

Relates to #11198
@clintongormley clintongormley changed the title Internal: count api to become a shortcut to the search api Count api to become a shortcut to the search api Jun 7, 2015
@clintongormley clintongormley added :Search/Search Search-related issues that do not fall into other categories and removed :Internal labels Jun 7, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>enhancement :Search/Search Search-related issues that do not fall into other categories v2.0.0-beta1
Projects
None yet
5 participants