GH-1246. Asynchronously initialize cache before reading

[kotman12]

Linked issue #1246

The current CachedModeledFrameworkImpl doesn't manage cache initialization for you. A perfect example of the kind of code you can expect to see in the wild is in the CachedModeledFramework tests themselves, i.e. blocking on semaphores that pin the reading thread to prevent it from certain operations that are cache dependent (but as far as I can tell exactly which operations are cache dependent is not really guaranteed so this is arguably cumbersome). Either way, this is fine in a lot of cases...

However, I propose an additional InitializedCachedModeledFramework implementation which asynchronously waits for the cache initialization trigger and only then proceeds to read from the cache. I implemented something similar for my personal use-case where I couldn't rely on, i.e. readThrough, to handle the uninitialized case because readThrough cannot disambiguate between a znode that is missing because it truly is missing from zk vs one that is missing because the cache hasn't initialized. In my case the znode wouldn't always exist in zk and so using readThrough would result in a lot of wasted calls to zk, greatly reducing the benefit of the cache in the first place.

To reiterate InitializedCachedModeledFramework has a couple benefits over the existing implementation:

1. No more possibility of misleading NoNodeException in the case of reading from CachedModeledFramework before the cache has warmed. (I say misleading because the node may exist .. just not in the cache)
2. No more temptation to add blocking semaphores in front of this non-blocking interface.
3. IMO generally less confusion about how to properly use this otherwise great(!) feature.

[tisonkun]

Thanks for your contribution @kotman12!

My extra consideration is whether the original manner has some benefits or we can even replace the old implement with the new one. Also compatibility is a consideration while we can bump a major version if needed and worthy.

Anyway, this patch itself LGTM.

[kotman12]

Thanks for taking a look @tisonkun! I think I agree .. IMO in a majority of cases the "initialized" variety is what you want even though there may be some minimal overhead of going through/checking two futures instead of one. I don't know at what scale that matters, if at all, but didn't benchmark it so didn't want to jump to conclusions.

I'd be happy to refactor CachedModeledFrameworkImpl itself instead of adding another implementation. A benefit of this approach is that tests would be a lot simpler but I'm not sure if this is something that needs to be benchmarked? At any rate, this shouldn't introduce any backwards incompatibilities so not sure a major version bump is even needed to change CachedModeledFrameworkImpl to manage initialization.

A third option is to make InitializedCachedModeledFramework the default cached implementation returned by ModeledFramework::cached. This would make the user-friendlier implementation more discoverable while also giving hardcore users the option to use the original, unitialized version.

Of course I'll defer to the more experienced community here.

[kotman12]

@tisonkun / @kezhuw please take a look at #1250 when you get a chance. I believe it is a less heavyweight implementation of the same thing we are doing here. Please don't merge this PR because while implementing #1250 I realized there are still gaps here. Rather than fix them I'll kindly first refer you to #1250 since if that is merged this one can be closed. I believe it should be backwards compatible and nearly identical from a performance standpoint.