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 Global Cache (memcache) #148

Merged
merged 6 commits into from
Aug 2, 2019
Merged

Conversation

chrisrossi
Copy link
Contributor

Does not include:

  • Concrete implementation of GlobalCache that uses Redis. (Should only take a day or so.)

  • use_datastore implementation.

Makes use of @takashi8 's surprise #130 contribution.

I still need to work on documentation, but this should be working now. This is extensive/tricky enough, that I'm likely to have overlooked something. Happy to have this one scrutinized fairly closely.

@chrisrossi chrisrossi requested review from cguardia and andrewsg July 30, 2019 20:51
@chrisrossi chrisrossi requested a review from crwilcox as a code owner July 30, 2019 20:51
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jul 30, 2019
Copy link
Contributor

@cguardia cguardia left a comment

Choose a reason for hiding this comment

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

Sorry it took some time to go through this.

@@ -142,8 +148,23 @@ def context(self, cache_policy=None):
cache_policy (Optional[Callable[[key.Key], bool]]): The
cache policy to use in this context. See:
:meth:`~google.cloud.ndb.context.Context.set_cache_policy`.
global_cache (Optional[_cache.GlobalCache]):
Copy link
Contributor

Choose a reason for hiding this comment

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

Might need to put the whole path here to appease sphinx fairly inconsistent reference finder.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's actually the fact that _cache isn't autodoc'ed yet. I plant to make a docs pass before merging this.

@andrewsg
Copy link
Contributor

Looks like the global cache access is happening in the datastore_api module, instead of in the user-facing functions like get and put. Is there a particular reason you made that decision?

@chrisrossi
Copy link
Contributor Author

Yes. The global cache stores serialized Datastore protocol buffer representations of entities. The _datastore_api layer stores and retrieves protocol buffer representations, so it made sense (to me) to put the global cache there where it's speaking the same language. This allows data to flow from low level Datastore protocol buffers to high level NDB entities in a single, straightforward pipeline.

@chrisrossi
Copy link
Contributor Author

HI @cguardia , I've rebased and added one more commit, if you want to take another look before I merge.

@chrisrossi chrisrossi merged commit 7ab8080 into googleapis:master Aug 2, 2019
@chrisrossi chrisrossi deleted the memcache branch January 12, 2020 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants