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

Indices stats options #6390

Closed
wants to merge 10 commits into from

Conversation

Projects
None yet
4 participants
@clintongormley
Copy link
Member

clintongormley commented Jun 3, 2014

This PR fixes two bugs in indices stats:

  • groups and types were being ignored
  • completion_fields with wildcards were not resolving the real field names

It also allows groups and types to accept wildcards, like fields, completion_fields and fielddata_fields

Added YAML tests for indices stats for: indices, metrics, level, fields, types, groups

clintongormley added some commits May 31, 2014

Stats: Indices stats ignoring "groups" and "types"
The "groups" and "types" parameters to indices stats were being set too
early, so could end up being cleared.
Stats: Make "groups" and "types" accept wildcards
The "fields" parameter to indices stats accepts wildcards, but
the "groups" and "types" parameters don't.
Added indices.stats YAML tests for:
* metrics
* level
* fields
* groups
* types
@clintongormley

This comment has been minimized.

Copy link
Member Author

clintongormley commented Jun 3, 2014

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 }

This comment has been minimized.

Copy link
@bleskes

bleskes Jun 4, 2014

Member

I'm not sure we assume we have 20 shards in the index?

This comment has been minimized.

Copy link
@clintongormley

clintongormley Jun 4, 2014

Author Member

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.

This comment has been minimized.

Copy link
@bleskes

bleskes Jun 4, 2014

Member

I think we randomize these in java land. If we don't do it now, the day is not far away..

This comment has been minimized.

Copy link
@javanna

javanna Jun 4, 2014

Member

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 ;)

This comment has been minimized.

Copy link
@clintongormley

clintongormley Jun 5, 2014

Author Member

Good to know - I've pushed a change to make the tests more determinative in this case :)

@@ -307,7 +307,7 @@ public CompletionStats stats(String... fields) {
// support for getting fields by regex as in fielddata
if (Regex.simpleMatch(field, entry.getKey())) {
long fstSize = entry.getValue().fst.sizeInBytes();
completionFields.addTo(field, fstSize);
completionFields.addTo(entry.getKey(), fstSize);

This comment has been minimized.

Copy link
@bleskes

bleskes Jun 4, 2014

Member

good catch

typesSt.put(type, statsHolder.stats());
for (Map.Entry<String, StatsHolder> entry : typesStats.entrySet()) {
for (String type : types) {
if (Regex.simpleMatch(type, entry.getKey())) {

This comment has been minimized.

Copy link
@bleskes

bleskes Jun 4, 2014

Member

This is a variant of Regex.simpleMatch that takes an array as the first parameter. This will simplify this code.

This comment has been minimized.

Copy link
@clintongormley

clintongormley Jun 4, 2014

Author Member

Oh cool - I'll change to that version.

groupsSt.put(group, statsHolder.stats());
for (Map.Entry<String, StatsHolder> entry : groupsStats.entrySet()) {
for (String group : groups) {
if (Regex.simpleMatch(group, entry.getKey())) {

This comment has been minimized.

Copy link
@bleskes

bleskes Jun 4, 2014

Member

Same here. Can be made simpler by using another varian of Regex.simpleMatch

- is_false: _all.primaries.search.groups

---
"Groups - one":

This comment has been minimized.

Copy link
@bleskes

bleskes Jun 4, 2014

Member

I think we need at least one metric type to trigger the bug, no? maybe add that to the tests?

This comment has been minimized.

Copy link
@clintongormley

clintongormley Jun 4, 2014

Author Member

Actually no. The default of all also clears existing flags, but I'll add a test with a metric as well.

This comment has been minimized.

Copy link
@bleskes

bleskes Jun 4, 2014

Member

you're right. Didn't expect IndicesStatsRequest.all() to clear the types and groups as it talks about flags.

@bleskes

This comment has been minimized.

Copy link
Member

bleskes commented Jun 4, 2014

Looking good! Left some comments

clintongormley added some commits Jun 4, 2014

Replaced for loop with Regex.simpleMatch with a variant which accepts…
… an array

for completion fields, fielddata fields, types and groups
@clintongormley

This comment has been minimized.

Copy link
Member Author

clintongormley commented Jun 4, 2014

@bleskes I've updated the PR in response to your comments.

@bleskes

This comment has been minimized.

Copy link
Member

bleskes commented Jun 4, 2014

@clintongormley changes look good. What about the number of shards tests?

@clintongormley

This comment has been minimized.

Copy link
Member Author

clintongormley commented Jun 5, 2014

@bleskes pushed changes to make the number of shards predetermined.

@bleskes

This comment has been minimized.

Copy link
Member

bleskes commented Jun 5, 2014

LGTM!

@javanna

This comment has been minimized.

Copy link
Member

javanna commented Jun 5, 2014

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.

@s1monw

This comment has been minimized.

Copy link
Contributor

s1monw commented Jun 5, 2014

+1 to what luca said! I think we should push towards java tests. Can we add those before pushing?

clintongormley added some commits Jun 6, 2014

@clintongormley

This comment has been minimized.

Copy link
Member Author

clintongormley commented Jun 6, 2014

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();

This comment has been minimized.

Copy link
@javanna

javanna Jun 10, 2014

Member

you can use the common method refresh() here which checks also if there are failures in the response.

client().prepareIndex("test1", "bar", Integer.toString(1)).setSource("{\"bar\":\"bar\",\"baz\":\"baz\"}").execute().actionGet();
client().prepareIndex("test1", "baz", Integer.toString(1)).setSource("{\"bar\":\"bar\",\"baz\":\"baz\"}").execute().actionGet();

client().admin().indices().prepareRefresh().execute().actionGet();

This comment has been minimized.

Copy link
@javanna

javanna Jun 10, 2014

Member

refresh()

.addMapping(
"bar",
"{ \"properties\": { \"bar\": { \"type\": \"string\", \"fields\": { \"completion\": { \"type\": \"completion\" }}},\"baz\": { \"type\": \"string\", \"fields\": { \"completion\": { \"type\": \"completion\" }}}}}")
.execute().actionGet();

This comment has been minimized.

Copy link
@javanna

javanna Jun 10, 2014

Member

if you use prepareCreate("test1") without the client().admin().indices() prefix you end up using a bit more randomization, since the number of shards is randomized in indexSettings() instead of taken from the index template.
Also you can then remove setSettings(indexSettings()).
And even better if you wrap the whole thing into assertAcked e.g. assertAcked(prepareCreate("test1").addMapping(....))


client().prepareIndex("test1", "bar", Integer.toString(1)).setSource("foo","bar").execute().actionGet();

client().admin().indices().prepareRefresh().execute().actionGet();

This comment has been minimized.

Copy link
@javanna

javanna Jun 10, 2014

Member

refresh()

client().prepareIndex("test1", "bar", Integer.toString(1)).setSource("foo","bar").execute().actionGet();
client().prepareIndex("test2", "baz", Integer.toString(1)).setSource("foo","bar").execute().actionGet();

client().admin().indices().prepareRefresh().execute().actionGet();

This comment has been minimized.

Copy link
@javanna

javanna Jun 10, 2014

Member

refresh()

@clintongormley

This comment has been minimized.

Copy link
Member Author

clintongormley commented Jun 10, 2014

@javanna thanks for looking at the tests. I've pushed a new commit which implements your changes.

@javanna

This comment has been minimized.

Copy link
Member

javanna commented Jun 10, 2014

Great work @clintongormley !!!!

clintongormley added a commit that referenced this pull request Jun 10, 2014

Stats: Bugfixes and enhancements to indices stats API
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

karmi added a commit to elastic/elasticsearch-ruby that referenced this pull request Jun 11, 2014

@s1monw s1monw removed the review label Jun 18, 2014

@clintongormley clintongormley changed the title Indices stats options Admin: Indices stats options Jul 16, 2014

@clintongormley clintongormley changed the title Admin: Indices stats options Indices stats options Jun 7, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.