Limit guava caches to 31.9GB #6286
Conversation
| logger.warn("reducing requested filter cache size of {} to the maximum allowed size of {}", new ByteSizeValue(sizeInBytes), | ||
| ByteSizeValue.MAX_GUAVA_SIZE); | ||
| sizeInBytes = ByteSizeValue.MAX_GUAVA_SIZE.bytes(); | ||
| } |
There was a problem hiding this comment.
I think you should store sizeInBytes in a local variable first so that no other thread can ever see a value that is greater than the maximum size?
|
I put the limit directly into ByteSizeValue because I couldn't really think of a better place for it. I also couldn't think of a good unit test for it. I did review guava and concur with the overflow. It looks to me like you need to give it enough room so that it doesn't overflow the int while it is adding things. I figured 100MB would do. |
| * evicted by size. We set this to 31.9GB leaving 100MB of headroom to | ||
| * prevent overflow. | ||
| */ | ||
| public static final ByteSizeValue MAX_GUAVA_SIZE = new ByteSizeValue(32 * ByteSizeUnit.C3 - 100 * ByteSizeUnit.C2); |
There was a problem hiding this comment.
Could the contant name be a bit more specific? eg. MAX_GUAVA_CACHE_SIZE?
There was a problem hiding this comment.
I had called it MAX_CACHE_SIZE then thought I'd make it MAX_GUAVA_CACHE_SIZE but my fingers didn't agree..... Fixing.
|
Something that would be nice would be to have an assertion or a unit test somewhere that ensures that there is an overflow happening in the guava cache so that we don't forget to remove this work-around when the upstream bug is fixed. |
|
I've just realized that in both cases there is a string called "size" that is a companion to the sizeInBytes that I modify. In both cases I don't modify the string. That feels funky. |
Sure. Let me do that. |
|
Added test case and "fixed" the size string every time I changed the size in bytes. |
| ByteSizeValue.MAX_GUAVA_CACHE_SIZE); | ||
| sizeInBytes = ByteSizeValue.MAX_GUAVA_CACHE_SIZE.bytes(); | ||
| } | ||
| this.size = ByteSizeValue.MAX_GUAVA_CACHE_SIZE.toString(); |
There was a problem hiding this comment.
I assume you wanted to put this assignment under the if statement. But I don't think the size as a String should be updated: it is used in the ApplySettings class in order to know whether the value has been updated, and when it has been updated, the cache needs to be rebuilt. If we set the size here, I'm afraid we would rebuild the cache everytime new settings would be applied, even if the value didn't change?
There was a problem hiding this comment.
Yes to all that and amending.
|
Left one comment about the fact that we should probably not update the (String) size in |
Guava's caches have overflow issues around 32GB with our default segment count of 16 and weight of 1 unit per byte. We give them 100MB of headroom so 31.9GB. This limits the sizes of both the field data and filter caches, the two large guava caches. Closes elastic#6268
|
Just updated. |
|
Merged, thanks! |
Guava's caches have overflow issues around 32GB with our default segment
count of 16 and weight of 1 unit per byte. We give them 100MB of headroom
so 31.9GB.
This limits the sizes of both the field data and filter caches, the two
large guava caches.
Closes #6268