Move caching of the size of a directory to StoreDirectory.

[jpountz]

In spite of the existing caching, I have seen a number of nodes hot threads
where one thread had been spending all its cpu on computing the size of a
directory. I am proposing to move the computation of the size of the directory
to StoreDirectory in order to skip recomputing the size of the directory if
no changes have been made. This should help with users that have read-only
indices, which is very common for time-based indices.

[elasticmachine]

Pinging @elastic/es-distributed

[s1monw]

I have some issues with this change:

• we introduced a time-based cache since some stats-endpoint hammered this which caused slowdowns
• the stats didn't need to be accurate

this change will bring back the problems we tried to solve since it will re-calculate once stuff has changed. I agree that it is only on live shards but still. I think we should stick with the time-based model but also have some indicator to skip-recalculation once we have an accurate number. ie. can we have some mod-count on createOutput and friends as well as once the stream is closed. and on deleteFile that way we can skip the re-calculation on non-live shards and are still protected from hammering this output.

The other option would be to move the cache out of Store entirely and cache on the IndexShard level. That way we can skip recalculation if the shard is idle. that would make most sense to me. WDYT?

[jpountz]

@s1monw This PR actually implements your first suggestion already: it has kept the time-based model via SingleObjectCache and only allows to skip even more store size computations on read-only shards by checking the mod count. So basicaly, the store size is only recomputed if the content of the directory has changed AND if more than refresh_interval millis elapsed.

I think the store is a better place for this than IndexShard since we care about inactivity in terms of written bytes as opposed to indexed documents, and background merges could cause variations of the store size on a shard that hasn't indexed any new documents?

[s1monw]

I think the store is a better place for this than IndexShard since we care about inactivity in terms of written bytes as opposed to indexed documents, and background merges could cause variations of the store size on a shard that hasn't indexed any new documents?

I still think the complexity here is very high for a minimal impact can we try to make this simpler?

[jpountz]

Why do you think the impact will be minimal? My reasoning was that if you have daily indices with a 30 days retention policy, 5 shards for new indices which are shrunk to 1 shard when they become read-only. Then you would have 5 read-write shards and 29 read-only shards, so such a change would reduce the number of reads of file attributes by about 7x since we would only need to compute the store size of 5 shards every 10 seconds instead of 34.

[ywelsch]

I also believe the impact to be non-negligible here, especially for cases where there are many (inactive) shards on a node. Regarding code complexity, I think it's possible to detach the modification counting directory from the cache, see https://github.com/elastic/elasticsearch/compare/master...ywelsch:mod-count-directory?expand=1 as an alternative. Maybe you like this better, @s1monw?

[s1monw]

Why do you think the impact will be minimal? My reasoning was that if you have daily indices with a 30 days retention policy, 5 shards for new indices which are shrunk to 1 shard when they become read-only. Then you would have 5 read-write shards and 29 read-only shards, so such a change would reduce the number of reads of file attributes by about 7x since we would only need to compute the store size of 5 shards every 10 seconds instead of 34.

sorry I mean code-impact. I totally see the point of your change. I should have been more clear. What I had in mind is something that doesn't increment a counter on every little byte write. The granularity is too much for he benefit. I would rather want to implement this on a Shard level and ask the engine if there are any pending merges or in-flight merges are happening ie.

if (isActive.get() || engine.hasRunningOrPendingMerges()) {
  doRefresh();
}

Also these stats are shard level stats I think that is the right place for it?

[jpountz]

@s1monw Would you rather like something along the lines of #30817?

[jpountz]

After further discussion with @s1monw I tried to move this option forward, but to remove the write to an AtomicLong in every writeByte(s) call. @s1monw Can you take another look?

[s1monw]

LGTM

[s1monw]

@jpountz wanna push this?

[jpountz]

For the record, this change doesn't look like it made indexing slower: https://elasticsearch-benchmarks.elastic.co/index.html#tracks/nyc-taxis/nightly/30d.