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

Initial implementation of request cache #223

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rkuris
Copy link
Contributor

@rkuris rkuris commented Sep 5, 2017

Still needs some docs, and we need to include this automatically with NewClient, but that will be done under a different PR.

@rkuris rkuris added this to the 2.4.0 milestone Sep 5, 2017
@rkuris rkuris self-assigned this Sep 5, 2017
@rkuris rkuris force-pushed the rkuris-request-cache branch 2 times, most recently from a8a7192 to 9b02478 Compare September 26, 2017 08:27

// CacheableContext produces a context that has a cache in it
// from one that does not
func CacheableContext(ctx context.Context) context.Context {
Copy link
Contributor

Choose a reason for hiding this comment

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

tl;dr I don't like this, but I can't think of a better solution, so let's go with this for now.

I think this is the one big problem I have with this design. In the application code, where do we call this function? I probably have to call it in my handler code, immediately when I first receive the request. That's what I'd have to do in order for this context to be used over the entire duration of the request. But now I've got a call to my Datastore logic all the way up in my handler. That seems like a leaky abstraction. If I change my datastore implementation, I'm now going to have to change my handler logic. This tightly couples the business logic to DOSA.

Also, It's not immediately obvious to me that if I'm using the RequestCacheConnector that I also need to use this special context creation function (or else the connector doesn't do anything). This violates the Principle of Least Astonishment.

The context, when it first arrives in the handler, is really the only thing whose lifecycle appropriately parallels the lifecycle of the request. I think what I'd like ideally is that the first time the connector sees the context, it initiallizes the cache and attaches it to the context. Then, in all subsequent requests the connector will see that a cache has been created and just use the existing one on the context. But I don't think that's possible given the way Context interface works.

@CLAassistant
Copy link

CLAassistant commented Jun 13, 2020

CLA assistant check
All committers have signed the CLA.

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.

3 participants