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
Indices stats options #6390
Indices stats options #6390
Conversation
The "groups" and "types" parameters to indices stats were being set too early, so could end up being cleared.
The "fields" parameter to indices stats accepts wildcards, but the "groups" and "types" parameters don't.
* metrics * level * fields * groups * types
I've added YAML tests rather than Java test - are Java tests still required, even if they just duplicate the YAML tests? |
- do: | ||
indices.stats: {} | ||
|
||
- match: { _shards.total: 20 } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure we assume we have 20 shards in the index?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes we can. I'm checking the total number of defined shards, not active shards. The test creates two indices with default settings == 20 shards.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we randomize these in java land. If we don't do it now, the day is not far away..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do randomize it....just run the tests and you'll see failures mvn clean test -Dtests.class=*.ElasticsearchRestTests
or just run the ElasticsearchRestTests
test form your IDE ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to know - I've pushed a change to make the tests more determinative in this case :)
Looking good! Left some comments |
… an array for completion fields, fielddata fields, types and groups
@bleskes I've updated the PR in response to your comments. |
@clintongormley changes look good. What about the number of shards tests? |
@bleskes pushed changes to make the number of shards predetermined. |
LGTM! |
My two cents on testing: the REST bug (groups and types were being ignored) can only be tested from a REST test. Beyond that, I'd love to see some Java test for these changes, although I do see that we have a very good coverage in terms of yaml tests. The reason is simply that we should not rely too much on the REST tests which are a bit of an afterthought and don't necessarily use all the randomizations that we use in our integration tests. Ideally we should be able to test the core without client tests as much as possible, and have a good coverage through Java tests. |
+1 to what luca said! I think we should push towards java tests. Can we add those before pushing? |
Found a bug in the YAML tests where, thanks to randomization, primaries may not reflect, eg positive query count. Fixed that and added Java tests |
client().prepareIndex("test1", "type2", Integer.toString(1)).setSource("field", "value").execute().actionGet(); | ||
client().prepareIndex("test2", "type", Integer.toString(1)).setSource("field", "value").execute().actionGet(); | ||
|
||
client().admin().indices().prepareRefresh().execute().actionGet(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can use the common method refresh()
here which checks also if there are failures in the response.
@javanna thanks for looking at the tests. I've pushed a new commit which implements your changes. |
Great work @clintongormley !!!! |
Bugs: * "groups" and "types" were being ignored * "completion_fields" as wildcards were not being resolved to fieldnames Enhancements: * Made "groups" and "types" support wildcards * Added missing tests Closes #6390
This PR fixes two bugs in indices stats:
groups
andtypes
were being ignoredcompletion_fields
with wildcards were not resolving the real field namesIt also allows
groups
andtypes
to accept wildcards, likefields
,completion_fields
andfielddata_fields
Added YAML tests for indices stats for: indices, metrics, level, fields, types, groups