bazel-buildfarm icon indicating copy to clipboard operation
bazel-buildfarm copied to clipboard

Fix expensive map size calculation via redisson

Open luxe opened this issue 3 years ago • 2 comments

Problem:

Calculating size of JedisMap is too expensive (see issue: (https://github.com/bazelbuild/bazel-buildfarm/issues/762). Below is an explanation of the problem, and why the solution was chosen.

Our map data is laid out in redis as standalone keys. They are conceptually grouped together through key prefixes.

{map-name}:key1 -> value
{map-name}:key2 -> value
{map-name}:key3 -> value

Because they are standalone keys, there is no way to calculate the conceptual size of the map without performing an O(N) scan. Additionally, the elements need loaded into memory because scans are done on different cluster nodes which will result in duplicates. We wouldn't be able to get proper size without de-duplicating. This does not scale in memory or time.

Idea 1:

Track the size ourselves. Increment and decrement a counter for insertions and deletions. This could be done in a distributed fashion as redis supports atomic counters via INCR / DECR. But this actually can't work. Don't forget that the keys have a TTL and they will expire. We could set up an event handler on expirations, but it gets messy.

Idea 2:

Use a better redis data structure. The one that makes the most sense is hashes. It's essentially a HashMap and it can function in the same way as our existing Map without the need for the prefix. The data structure itself is what groups the keys together. It acts like 1 key with multiple field/values which supports HLEN for getting the size in O(1). This won't work either.
Redis hashes don't support TTL which is a requirement for this data. https://github.com/redis/redis/issues/167 https://github.com/redis/redis/issues/1042

Idea 3:

Use Redisson because they've already worked around these limitations. See their documentation note:

Map-based cache with ability to set TTL for each entry via put(...)

Current redis implementation doesnt have map entry eviction functionality. Thus entries are checked for TTL expiration during any key/value/entry read operation. Expired tasks cleaned by EvictionScheduler. This scheduler deletes expired entries in time interval between 5 seconds to 2 hours.

If eviction is not required then it's better to use RedissonMap.

Code:

We replace JedisMap with a Redisson implementation which has correct size() performance. The CasWokerMap is a multimap and already solved this problem with a config option.

luxe avatar Apr 24 '21 06:04 luxe

Does this address the previously slow implementation of OperationsStatus? If so, I'm all for it, while still providing the same information...

werkt avatar Aug 07 '21 21:08 werkt

Does this address the previously slow implementation of OperationsStatus? If so, I'm all for it, while still providing the same information...

yes, it fixes all the size calculations from doing scans. so they will be instant. But I'll have to adjust this so redis vs redisson can be chosen by configuration. Because I don't know how these redisson maps behave on a large production deployment

luxe avatar Aug 11 '21 03:08 luxe