Leave a single CachePopulator and choose sync/async mode automatically

[leventov]

In this issue, I list several ideas regarding refactoring/improvement of CachePopulator subsystem.

1. It seems to me that BackgroundCachePopulator and ForegroundCachePopulator would be much more intuitively called Sync/Async ("background cache population" and "foreground cache population" terms in docs should then be updated to "synchronous/asynchronous cache population").

2. There could be a single implementation. The executor may be DirectExecutorService in the "sync" case.

3. It seems to me that the crucial difference between sync and async is whether blocking cache (Redis/Memcached) or local, (mostly) non-blocking cache (Caffeine) is used. At least, this should be highlighted in docs; at most, the mode of cachePopulator should be chosen automatically depending on the configured type of the cache.

4. Does it make any sense to make async cache populator which pushes results to remote cache to have more than 1 thread? After all, we will be hitting the same bottleneck - the local NIC. So async populator may just have a queue and a single thread takes cache entries from the queue and tries to push them to remote cache.

[gianm]

Maybe it's just me, but I find foreground/background and sync/async to be equally understandable.

There could be a single implementation. The executor may be DirectExecutorService in the "sync" case.

A note: at least currently, the control flow is different enough that the foreground populator cannot be implement as a background populator with a direct executor service. I think the main reason is that the background populator isn't only trying to do the population in the background, it's also trying to do computation of cache entries in the background and in parallel.

The latter thing means that it needs to take a different approach to the foreground one. In particular, the foreground populator is simple: it writes something to a ByteArrayOutputStream for each item as the Sequence is walked. The background populator instead writes these items into a List and then starts up separate thread pool tasks to serialize each one, and then also populates the cache using yet another task.

In other words, if the foreground populator was replaced with a background populator + direct executor, it would use a lot more memory than it does today, since it would need to buffer up a List of items that it currently just writes immediately to a ByteArrayOutputStream.

It seems to me that the crucial difference between sync and async is whether blocking cache (Redis/Memcached) or local, (mostly) non-blocking cache (Caffeine) is used. At least, this should be highlighted in docs; at most, the mode of cachePopulator should be chosen automatically depending on the configured type of the cache.

It's not necessarily that simple, for two reasons:

1. Some remote cache implementations (like memcached) queue things internally and return from cache.put before the remote server acknowledges the write. So background population doesn't add any extra resistance to network based blocking.
2. Even for local cache implementations, background population could in theory be useful, since it does cache entry generation in the background too. This could in theory be expensive enough (CPU-wise) that it is useful to background it.

Does it make any sense to make async cache populator which pushes results to remote cache to have more than 1 thread? After all, we will be hitting the same bottleneck - the local NIC. So async populator may just have a queue and a single thread takes cache entries from the queue and tries to push them to remote cache.

It depends how exactly the Cache impl works. If its put method is blocking, then having more than one thread is useful, since in that case you can get more bandwidth by doing multiple connections requests in parallel (otherwise, you have too much 'dead air' while waiting for responses). If its put method is CPU intensive (maybe it must wrap the provided byte[] in an envelope of sorts, involving a lot of memory copies) then having more than one thread could also improve throughput (but maybe a bad idea, since it's scary if you're spending so much CPU on putting stuff into the query cache). Otherwise, it's probably not useful to have more than one thread.

[gianm]

By the way, after writing the above comment, now I'm starting to wonder if the design of the background populator makes sense. In particular, is it really a good idea to try to compute cache entries asynchronously and in parallel? And is it a good idea to have background cache population be a generic thing at all? What if we only had foreground population and left it up to the individual Cache impls to do background pushing to remote cache servers if they want to?

[leventov]

And is it a good idea to have background cache population be a generic thing at all? What if we only had foreground population and left it up to the individual Cache impls to do background pushing to remote cache servers if they want to?

This is very similar to the automatic sync/async choice that I suggested. It makes sense to me, too.

[himanshug]

is it really a good idea to try to compute cache entries asynchronously and in parallel?

I think, no . it is ok to compute the entry in the same thread reading sequence.

is it a good idea to have background cache population be a generic thing at all? What if we only had foreground population and left it up to the individual Cache impls to do background pushing to remote cache servers if they want to?

it makes sense to leave that choice to cache impl instead of user, but all remote caches would need to do it in the background and would end up writing same type of code. it will be useful for that common code to be made available in druid core so that remote cache extensions could just use it.