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

Implement cache policy contract for ODSP driver #2282

Closed
wants to merge 1 commit into from
Closed

Implement cache policy contract for ODSP driver #2282

wants to merge 1 commit into from

Conversation

christiango
Copy link
Member

As we start looking into improving performance for boot scenarios via snapshot caching, it's become clear that the host app is going to need to provide input on how the cache is used, since each host may have different scenarios or preferences.

Principles:

  • Cache remains agnostic to host app. Host app should have a simple contract to implement and is mostly in the business of storing the data in a secure and compliant manner.
  • The host app is given settings it can use to customize caching behavior. The host app is in the best position to understand usage patterns and make the trade off between cache hit rate and staleness of content shown to the user.

@vladsud
Copy link
Contributor

vladsud commented May 22, 2020

With the functionality we have today, I'd rather see it implemented via one of these two ways:

  1. driver continues to supply its default policy, but host (implementer of cache) can override it. No need to expose it to driver at all.
  • modification of that - default driver policy moves to default cache implementation and we completely remove expiration from interface.
  1. If driver needs to know host policy, I'd rather expose it on cache interface, not pass it around as separate object. Host can always combine generic cache implementation and policy into one object (but keep implementations separate)

@vladsud
Copy link
Contributor

vladsud commented May 22, 2020

That said, the shape you propose may make sense if we keep adding stuff to policy interface, hard to say given unknowns

@christiango
Copy link
Member Author

@vladsud - The issue I have is that our cache has no idea of the context of what the driver is trying to save. For example, join session is not safe to save for many weeks, trees/latest might be, blobs definitely are.

@christiango
Copy link
Member Author

I'm fine putting it on the cache interface (though that's kind of weird) or having the driver pass more context on what is being asked to be stored.

@christiango
Copy link
Member Author

christiango commented May 22, 2020

I don't think # 1 is feasible, unless we update the cache API to give the cache implementer more context. I am open to # 2, even though that contract is a bit awkward to have the cache implementation and policy on the same interface

@vladsud
Copy link
Contributor

vladsud commented May 22, 2020

I think we will want to provide more context either way (like if you need to evict all data for given file, or prioritize some files over others.). How about I add that and we see where it leads us?

@vladsud
Copy link
Contributor

vladsud commented May 22, 2020

@markfields - FYI, can we get your PR in today to avoid conflicts in this area before I start poking here?

@christiango
Copy link
Member Author

So which would you prefer?

  1. Have host policy on ICache. This can be explicit, like saying how long you can cache user content, or defining policies that are driver specific, like telling the driver to clear cached content related to snapshots after 500 ops have passed since the sequence number on the snapshot. I think the complexity of caching should be in the driver, with input from the host on policy and things that depend on usage patterns.
  2. Pass in additional context in put, like saying what kind of info is being store

I think the first one is more flexible and provides more options.

@christiango
Copy link
Member Author

Sure, I am open to other approaches. The first issue we need to solve is the 10 second default right now. That should be our priority, giving host apps a way to specify they should cache longer.

I do want to keep the cache implementation simple, if we can.

@christiango
Copy link
Member Author

Closing since Vlad has offered to go about this in a better way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants