Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add withCachedTypes to HollowConsumer.Builder #379

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ghost
Copy link

@ghost ghost commented Jan 18, 2019

Almost all of the wiring was in place to pass a list of cached types,
just that there were no builder methods.

See https://hollow.how/advanced-topics/#caching for an overview of the
feature.

@ghost ghost requested review from PaulSandoz and DanielThomas January 18, 2019 01:18
@PaulSandoz PaulSandoz assigned ghost Jan 18, 2019
*
* @see <a href="https://hollow.how/advanced-topics/#caching">https://hollow.how/advanced-topics/#caching</a>
*/
public B withCachedTypes(String...cachedTypes) {
Copy link
Contributor

@PaulSandoz PaulSandoz Jan 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to specify this more clearly with regards to the client API. POJO is more of a producer term.

Jotting down some notes.

In this case we are referring to hollow records and the choice between using the HollowObjectFactoryProvider when caching is not enabled and the HollowObjectCacheProvider when caching is enabled. Such choice is performed by the generated API class (i.e. see HollowConsumer.Builder.withGeneratedAPIClass) which is instantiated by the HollowConsumer with the cached types, if any.

The generated API class will use one of the factories, HollowObjectFactoryProvider or HollowObjectCacheProvider (the latter effectively uses the former) to instantiate an instance of a HollowRecord for the type and cache it. The concrete type of the HollowRecord instance will be the generated API class associated with the type.

When an instance of a type is returned from the client API, for example lookup given its ordinal or via a search using an index, the factory will be used. When caching is enabled the same HollowRecord instance of a type will always be returned for a given ordinal (even if the consumer refreshes, assuming the ordinal has not been removed). (The caching is not sophisticated, such as using a LRU to keep the top N accessed records, perhaps using with soft references.)

Caching may be disabled by calling the detachCaches method on the generated API (although once disabled it cannot be re-enabled).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I originally wrote this as extra arguments to withGeneratedAPIClass, e.g.:

public CinderConsumerBuilder withGeneratedAPIClass(
    Class<? extends HollowAPI> generatedAPIClass,
    String...cachedTypes) { ... }

Would this be preferable, @PaulSandoz? It would reinforce that the two go hand-in-hand.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, an additional method with a varargs parameter would more closely bind the relationship.
We could make it:
withGeneratedAPIClass(Class<? extends HollowAPI> generatedAPIClass, String cacheType, String... additionalCachedTypes)
Thereby more clearly disambiguating the two method.

@ghost
Copy link
Author

ghost commented Jan 18, 2019

Related to #380 in the sense that users of the HollowConsumer API never had a chance to enable caching and thus never exposed to the thread safety issues. We may have seen issues caused by this within Netflix where we use lower-level APIs to enable the caching.

@ghost ghost force-pushed the feature/cached-types-consumer-builder branch from aab6d85 to 59a8bf2 Compare January 19, 2019 00:43
Tim Taylor added 2 commits January 21, 2019 17:23
Almost all of the wiring was in place to pass a list of cached types,
just that there were no builder methods.

See https://hollow.how/advanced-topics/#caching for an overview of the
feature.
@ghost ghost force-pushed the feature/cached-types-consumer-builder branch from 59a8bf2 to deca23c Compare January 22, 2019 23:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant