Return cached segments stats if include_unloaded_segments is true#39698
Return cached segments stats if include_unloaded_segments is true#39698s1monw merged 5 commits intoelastic:masterfrom
include_unloaded_segments is true#39698Conversation
Today we don't return segments stats for closed indices which makes it hard to tell how much memory such an index would require. With this change we return the statistics if requested by setting `include_unloaded_segments` to true on the rest request. Relates to elastic#39512/
s1monw
added
>enhancement
:Distributed/Engine
|
Pinging @elastic/es-distributed |
ywelsch
left a comment
ywelsch
left a comment
There was a problem hiding this comment.
Perhaps also add a REST test showing that these stats are properly working for closed replicated indices? We can also do that as a follow-up if you want to keep this an engine-level change only.
| } | ||
| return stats; | ||
| } else { | ||
| return super.segmentsStats(includeSegmentFileSizes, includeUnloadedSegments); |
There was a problem hiding this comment.
calling this will fail on a NoopEngine?
Should we instead return an empty stats object?
There was a problem hiding this comment.
hmm there is a test for this here that shows that it's not failing but returning an empty object.
I will add one. |
|
@ywelsch I pushed updates |
ywelsch
left a comment
ywelsch
left a comment
There was a problem hiding this comment.
I've left one more question. Looking good otherwise.
| "default" : "open", | ||
| "description" : "Whether to expand wildcard expression to concrete indices that are open, closed or both." | ||
| }, | ||
| "forbid_closed_indices": { |
There was a problem hiding this comment.
are we introducing this parameter only for BWC of behavior in 7.x or do you think this could still be useful in 8.0? In 8.0 I would expect this parameter to be false by default, and I'm not sure why I would ever explicitly set it to true.
There was a problem hiding this comment.
@ywelsch I must have missed something, the tests failed if I'd not have that in there. Maybe I ran tests before the relevant change was introduced?
There was a problem hiding this comment.
I think there is a misunderstanding. What I meant by "In 8.0 I would expect this parameter to be false by default" is that I think we should make it default to false in 8.0, so that if you include closed indices in your indices stats request, that the request does not fail by default.
There was a problem hiding this comment.
@ywelsch I would like to merge as is and then go and change the defaults for stats in a separate pr. ok with you?
There was a problem hiding this comment.
yes, let's do it like that
5 participants
Today we don't return segments stats for closed indices which makes it
hard to tell how much memory such an index would require. With this change
we return the statistics if requested by setting
include_unloaded_segmentstotrue on the rest request.
Relates to #39512