-
Notifications
You must be signed in to change notification settings - Fork 24.8k
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
Add periodic flush count to flush stats #29360
Conversation
Currently, a flush stats contains only the total flush which is the sum of manual flush (via API) and periodic flush (async triggered when the uncommitted translog size is exceeded the flush threshold). Sometimes, it's useful to know these two numbers independently. This commit tracks and returns a periodic flush count in a flush stats.
Pinging @elastic/es-core-infra |
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.
left one comment
@@ -85,6 +96,7 @@ public TimeValue getTotalTime() { | |||
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { | |||
builder.startObject(Fields.FLUSH); | |||
builder.field(Fields.TOTAL, total); | |||
builder.field(Fields.PERIODIC, periodic); |
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.
can we have a rest test that checks that this is there?
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.
@s1monw
I had a test but I could not verify the periodic value as the periodic flush is executed async. Do you have any suggestion for this? Or just check its presence (eg. 0 is ok)?
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 used the analogue of gt: { periodic: 0}
when I did something similar.
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.
Thanks @DaveCTurner. Unfortunately, sometimes a period flush might still be executing (or just scheduled) and periodic
is still 0.
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.
Apologies, I meant gte: { periodic: 0 }
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.
yeah ++
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 will go this suggestion. Thanks @DaveCTurner and @s1monw.
@s1monw I've added a rest test. Can you please have another look? Thank you! |
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.
LGTM
Thanks @s1monw and @DaveCTurner. |
Currently, a flush stats contains only the total flush which is the sum of manual flush (via API) and periodic flush (async triggered when the uncommitted translog size is exceeded the flush threshold). Sometimes, it's useful to know these two numbers independently. This commit tracks and returns a periodic flush count in a flush stats.
Currently, a flush stats contains only the total flush which is the sum
of manual flush (via API) and periodic flush (async triggered when the
uncommitted translog size is exceeded the flush threshold). Sometimes,
it's useful to know these two numbers independently. This commit tracks
and returns the periodic flush count in a flush stats.
Relates #29125