Fix wrong search stats groups in indices API #8950
Conversation
This provides a fix to issue elastic#7644.
| @@ -60,6 +60,10 @@ public Stats(long queryCount, long queryTimeInMillis, long queryCurrent, long fe | |||
| this.fetchCurrent = fetchCurrent; | |||
| } | |||
|
|
|||
| public Stats(Stats stats) { | |||
There was a problem hiding this comment.
if this new constructor is only used in the same class we can reduce its visibility perhaps?
There was a problem hiding this comment.
Maybe it could be used outside of the class at some point, so might be better to leave constructors public in general.
There was a problem hiding this comment.
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
|
Hey @alexksikes thanks for looking into this. Could you add some description to the PR that explains why this happens? I am also not clear yet on whether this would cause problems on the REST layer only or also on the transport layer (when the api is called through java api), maybe we can clarify that too. |
|
this is a good fix, I agree with @javanna that we shoud have a better comment why this happens? I also want a real unittest for this instead of a REST test. These REST test have a massive overhead and this one can be easily tested in a unittest |
| @@ -60,6 +60,10 @@ public Stats(long queryCount, long queryTimeInMillis, long queryCurrent, long fe | |||
| this.fetchCurrent = fetchCurrent; | |||
| } | |||
|
|
|||
| public Stats(Stats stats) { | |||
| this.add(stats); | |||
There was a problem hiding this comment.
wondering if this should call this(stats.queryCount, etc.), since the addition is not required given that we are creating a new object
There was a problem hiding this comment.
The vars are all set to zero so it would do the same thing.
There was a problem hiding this comment.
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.
|
The fix looks good, I left a couple of small comments, also I would add some unit tests for this, so that we won't need the REST test. Maybe also test group stats in the existing stats integration test. |
alexksikes
force-pushed
the
fix/search-stats-groups
branch
from
4ea87c5
to
be985c6
Compare
|
Thanks for the comments, I've added a unit test to show where the problem is exactly. I suppose I can remove the REST test? |
|
yeah please remove the REST test |
| checkStats(groupStats1.get("group1"), 3); | ||
| } | ||
|
|
||
| private void checkStats(Stats stats, long equalTo) { |
|
Left a few comments |
alexksikes
force-pushed
the
fix/search-stats-groups
branch
from
d50dc39
to
edf6456
Compare
|
LGTM |
This provides a fix to issue #7644. A new Stats object must be created, and not a reference to the retrieved stats, before we can add stats to it. Otherwise, we would keep on adding to the same object on subsequent calls to IndicesStatsResponse#getPrimaries() or IndicesStatsResponse#getTotal(). Closes #7644 and #8950
This provides a fix to issue #7644. A new Stats object must be created, and not a reference to the retrieved stats, before we can add stats to it. Otherwise, we would keep on adding to the same object on subsequent calls to IndicesStatsResponse#getPrimaries() or IndicesStatsResponse#getTotal(). Closes #7644 and #8950
This provides a fix to issue #7644. A new Stats object must be created, and not a reference to the retrieved stats, before we can add stats to it. Otherwise, we would keep on adding to the same object on subsequent calls to IndicesStatsResponse#getPrimaries() or IndicesStatsResponse#getTotal(). Closes #7644 and #8950
clintongormley
added
the
:Data Management/Stats
clintongormley
changed the title
This provides a fix to issue elastic#7644. A new Stats object must be created, and not a reference to the retrieved stats, before we can add stats to it. Otherwise, we would keep on adding to the same object on subsequent calls to IndicesStatsResponse#getPrimaries() or IndicesStatsResponse#getTotal(). Closes elastic#7644 and elastic#8950
This provides a fix to issue #7644. A new Stats object must be created, and not a reference to the retrieved stats, before we can add stats to it. Otherwise, we would keep on adding to the same object on subsequent calls to IndicesStatsResponse#getPrimaries() or IndicesStatsResponse#getTotal().