Cache Expensive Index Stats in TransportClusterStatsAction#63943
Cache Expensive Index Stats in TransportClusterStatsAction#63943original-brownbear merged 3 commits intoelastic:masterfrom original-brownbear:make-monitoring-less-crazy-expensive
Conversation
Follow up to #62753, which moved this expensive method off of the transport threads. Often times monitoring will hit this endpoint every few seconds while the metadata will likely change at a much slower interval. Given that in practice the computation of the stats might take up to a minute for large cluster states, caching them on a best-effort basis seems like a reasonable improvement.
|
Pinging @elastic/es-core-features (:Core/Features/Monitoring) |
| // guards #mappingStats, #analysisStats and #metaVersion | ||
| private final Object statsMutex = new Object(); | ||
|
|
||
| private MappingStats mappingStats; |
There was a problem hiding this comment.
Obviously this isn't sticky but in practice this will mostly be called on master by monitoring and the overhead of this is small. And even if this is less helpful for the stats REST API it's going to often help anyway statistically speaking I'd expect it to "often" help still.
|
|
||
| private MappingStats mappingStats; | ||
| private AnalysisStats analysisStats; | ||
| private long metaVersion = -1L; |
There was a problem hiding this comment.
I'm absolutely out of practice here, so please do correct me if I'm wrong, but should metaVersion be volatile?
There was a problem hiding this comment.
No need for that. We only ever touch that field under the statsMutex so we are guaranteed to always have a consistent view of it.
| MappingStats mappingStats = null; | ||
| AnalysisStats analysisStats = null; |
There was a problem hiding this comment.
Just a drive-by comment (I'll leave the full review to Dan), but can you rename these variables? I think shadowing class variables (outside of constructors) can lead to bugs for people who reference it not realizing which one they're getting
danhermann
left a comment
There was a problem hiding this comment.
Thanks, @original-brownbear! This LGTM once @dakrone's comment has been addressed.
|
Jenkins run elasticsearch-ci/1 (some unrelated reindex thingy) |
|
Thanks everyone! |
…64195) Follow up to #62753, which moved this expensive method off of the transport threads. Often times monitoring will hit this endpoint every few seconds while the metadata will likely change at a much slower interval. Given that in practice the computation of the stats might take up to a minute for large cluster states, caching them on a best-effort basis seems like a reasonable improvement.
Follow up to #62753, which moved this expensive method off of the transport threads.
Often times monitoring will hit this endpoint every few seconds while the metadata
will likely change at a much slower interval.
Given that in practice the computation of the stats might take up to a minute for large
cluster states, caching them on a best-effort basis seems like a reasonable improvement.