-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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 stats for active compactions #7281
Conversation
statTSMOptimizeCompactions = "tsmOptimizeCompactions" | ||
statTSMOptimizeCompactionsActive = "tsmOptimizeCompactionsActive" |
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.
Should we do something similar to the query executor for this? I remember for the query executor, we decided to add tracking how many compactions were run because it would end up being zero a lot. I know TSM compactions generally run faster so this might not be an issue for them, but this might run into the same issue where a compaction runs between monitor intervals and it never gets recorded.
Although we kept both for the query executor since one was more useful for SHOW STATS
. Maybe we should have both?
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.
Some (many? most?) compactions will run in less than the 10-second period between writes to _internal, but we've seen systems under heavy load that have had compactions running for several minutes. The difficulty in determining the cause of the change in system load without an indicator of active compactions, was the driving motivation in adding a stat for active compactions.
What do you mean by "something similar to the query executor"? Separate counters for started vs finished?
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.
Yea, that's what I was thinking. Separate counters for started vs finished. This could be fine by itself though.
be5a5de
to
930daeb
Compare
@jsternberg I moved all the logic around executing a compaction into a |
930daeb
to
4ea4485
Compare
|
||
if err == errCompactionInProgress { | ||
time.Sleep(time.Second) |
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.
@jwilder it seems like every error case has a time.Sleep
- but errCompactionsDisabled
seems like the only error case where we don't sleep.
Do you think it would be okay to remove the sleep calls from compactGroup
and unconditionally sleep inside compactTSMLevel
and compactTSMFull
?
TSMFullCompactions int64 | ||
TSMFullCompactionErrors int64 | ||
TSMFullCompactionDuration int64 | ||
CacheCompactions int64 // Counter of cache compactions that have ever run. |
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.
Documentation ❤️
2f7b08b
to
3573c83
Compare
c704140
to
882536f
Compare
@mark-rushakoff Looks good. Needs a rebase though. |
Unify logic around compaction execution to a single place. Also report on the error stats that we track. Previously they were not emitted in the stats output.
882536f
to
377c40f
Compare
Required for all non-trivial PRs
Adds stats to track the current number of active compactions. The existing stats for compactions were only updated after a compaction completed.
Also report on the error stats that we track. Previously they were not
emitted in the stats output.