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.

Sign up for GitHub

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

Jump to bottom

Fix wrong search stats groups in indices API #8950

Closed
wants to merge 4 commits into from
Closed

Fix wrong search stats groups in indices API #8950

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9f1abd8
Indices API: Fix wrong search stats groups
alexksikes Dec 15, 2014
be985c6
added unit test
alexksikes Dec 15, 2014
3924eb2
remove REST test and adressed comments
alexksikes Dec 16, 2014
edf6456
addressed comments
alexksikes Dec 16, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 29 additions & 0 deletions rest-api-spec/test/indices.stats/16_issue7644.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
---
setup:
- do:
indices.create:
index: test1
body:
settings:
number_of_shards: 2
number_of_replicas: 0

- do:
index:
index: test1
type: bar
id: 1
body: { "foo": "bar" }

- do:
search:
body:
stats: [ group1 ]

---
"Groups - shards query total":
- do:
indices.stats: { groups: _all, level: shards }

- match: { indices.test1.shards.0.0.search.groups.group1.query_total: 1 }
- match: { indices.test1.shards.1.0.search.groups.group1.query_total: 1 }
6 changes: 5 additions & 1 deletion src/main/java/org/elasticsearch/index/search/stats/SearchStats.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,10 @@ public Stats(long queryCount, long queryTimeInMillis, long queryCurrent, long fe
this.fetchCurrent = fetchCurrent;
}

public Stats(Stats stats) {
Copy link
Member

Choose a reason for hiding this comment

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

if this new constructor is only used in the same class we can reduce its visibility perhaps?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe it could be used outside of the class at some point, so might be better to leave constructors public in general.

Copy link
Member

Choose a reason for hiding this comment

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

I am not sure why...I wouldn't leave public if not needed. Actually I am even thinking that we could remove this constructor and just do new Stats(stats.queryCount etc.) directly, which would address my other comment too

this.add(stats);
Copy link
Member

Choose a reason for hiding this comment

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

wondering if this should call this(stats.queryCount, etc.), since the addition is not required given that we are creating a new object

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The vars are all set to zero so it would do the same thing.

Copy link
Member

Choose a reason for hiding this comment

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

I know the result is the same, but we are not adding to something existing, we are creating a new object. The addition is not needed and I think it would make the code cleaner if we called the other constructor.

}

public void add(Stats stats) {
queryCount += stats.queryCount;
queryTimeInMillis += stats.queryTimeInMillis;
Expand Down Expand Up @@ -178,7 +182,7 @@ public void add(SearchStats searchStats, boolean includeTypes) {
for (Map.Entry<String, Stats> entry : searchStats.groupStats.entrySet()) {
Stats stats = groupStats.get(entry.getKey());
if (stats == null) {
groupStats.put(entry.getKey(), entry.getValue());
groupStats.put(entry.getKey(), new Stats(entry.getValue()));
} else {
stats.add(entry.getValue());
}
Expand Down