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.

Sign up for GitHub

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

Jump to bottom

Implemented automatic caching for the discovery documents. #127

Merged
merged 1 commit into from
Aug 24, 2015
Merged

Implemented automatic caching for the discovery documents. #127

merged 1 commit into from
Aug 24, 2015

Conversation

tmatsuo
Copy link
Contributor

@tmatsuo tmatsuo commented Aug 19, 2015

Added 2 keyword arguments; cache_discovery and cache. You can suppress caching by setting cache_discovery=False. You can inject your own caching module via the cache kwarg.

It automatically uses app engine memcache if it's detected. It's not a hard dependency. Then fall back to file based caching.

fixes #110

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Aug 19, 2015
@tmatsuo
Copy link
Contributor Author

tmatsuo commented Aug 19, 2015

@nathanielmanistaatgoogle I'd appreciate it if you could take a look.

@tmatsuo
Copy link
Contributor Author

tmatsuo commented Aug 20, 2015

@dhermes FYI

@dhermes
Copy link
Contributor

dhermes commented Aug 20, 2015

Wow. So happy this is coming (3 years later)!

/cc @wescpy This is what we talked about at SFPython

nathanielmanistaatgoogle
nathanielmanistaatgoogle reviewed Aug 20, 2015
View reviewed changes
googleapiclient/discovery.py
@@ -171,6 +173,9 @@ def build(serviceName,
request.
credentials: oauth2client.Credentials, credentials to be used for
authentication.
cache_discovery: Boolean, whether or not cache the discovery doc.

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@nathanielmanistaatgoogle
Copy link
Contributor

Please squash commits.

@tmatsuo
Copy link
Contributor Author

tmatsuo commented Aug 20, 2015

Thanks for the comment @nathanielmanistaatgoogle

All addressed, PTAL

@tmatsuo
Copy link
Contributor Author

tmatsuo commented Aug 20, 2015

Let me write some tests. I'll update here once done.

@tmatsuo
Copy link
Contributor Author

tmatsuo commented Aug 21, 2015

I added some tests in test_discovery. PTAL

nathanielmanistaatgoogle
nathanielmanistaatgoogle reviewed Aug 21, 2015
View reviewed changes
googleapiclient/discovery.py
Args:
url: string, the URL of the discovery document.
http: httplib2.Http, An instance of httplib2.Http or something that acts
like it that HTTP requests will be made through.

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

nathanielmanistaatgoogle
nathanielmanistaatgoogle reviewed Aug 23, 2015
View reviewed changes
googleapiclient/discovery_cache/file_cache.py
"""File based cache for the discovery document.
The cache is stored in a single file so that multiple processes can

share the same cache. It locks the file whenever accesing to the

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@nathanielmanistaatgoogle
Copy link
Contributor

@dhermes are you also reviewing this or just excited about it happening?

@dhermes
Copy link
Contributor

dhermes commented Aug 23, 2015

I am not reviewing.

@tmatsuo
Copy link
Contributor Author

tmatsuo commented Aug 23, 2015

@nathanielmanistaatgoogle
I addressed your comments. PTAL

@nathanielmanistaatgoogle
Copy link
Contributor

Looks good. Are there more unit tests still to come or is this what you'd like to have merged?

@tmatsuo
Copy link
Contributor Author

tmatsuo commented Aug 23, 2015

@nathanielmanistaatgoogle Thanks!
I would like to add some unit tests for the cache module. I will likely write those early next week.

@nathanielmanistaatgoogle
Copy link
Contributor

Great; I'll hold steady until I hear more from you.

@tmatsuo tmatsuo force-pushed the master branch 2 times, most recently from 782ba62 to 1e10bfa Compare August 24, 2015 17:33
@tmatsuo
Copy link
Contributor Author

tmatsuo commented Aug 24, 2015

I have just added a unit test for the file cache, which discovered several bugs in my code!

@nathanielmanistaatgoogle Now I'm confident to say "let's merge this". PTAL

nathanielmanistaatgoogle
nathanielmanistaatgoogle reviewed Aug 24, 2015
View reviewed changes
tests/test_discovery_cache.py
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

nathanielmanistaatgoogle added a commit that referenced this pull request Aug 24, 2015
@nathanielmanistaatgoogle
Merge pull request #127 from tmatsuo/master
ed3d5a6 
Implemented automatic caching for discovery documents.
@nathanielmanistaatgoogle nathanielmanistaatgoogle merged commit ed3d5a6 into googleapis:master Aug 24, 2015
@nathanielmanistaatgoogle
Copy link
Contributor

Thanks for the contribution!

@tmatsuo
Copy link
Contributor Author

tmatsuo commented Aug 24, 2015

Thanks for the review! I'm wondering when it goes to pypi distribution so that everyone can benefit from it.

@nathanielmanistaatgoogle
Copy link
Contributor

Likely not this week; we're swamped with other things. Perhaps toward the end of next week? Would that be acceptable? Otherwise "sometime in September"?

@tmatsuo
Copy link
Contributor Author

tmatsuo commented Aug 24, 2015

Yeah, sounds good, thanks

@theacodes theacodes mentioned this pull request Sep 2, 2015
Closed
@tmatsuo
Copy link
Contributor Author

tmatsuo commented Sep 3, 2015

Ok, will you be able to do the release this week? If you still have other things, maybe I can do the release, especially if there's how-to doc. Let me know if you want me to do.

@tmatsuo tmatsuo mentioned this pull request Sep 3, 2015
Merged
@tmatsuo
Copy link
Contributor Author

tmatsuo commented Sep 10, 2015

To clarify, I have uploaded a small library to pypi before, so I know the basic. My username on pypi is tmatsuo, so maybe you can add me to the owners list of this package on pypi so that I can do the release?

My current guess for how to release is that:

  1. Create a release on github
  2. upload the package to testpypi first and do the sanity check
  3. upload it to pypi

@tmatsuo
Copy link
Contributor Author

tmatsuo commented Sep 14, 2015

JFYI it's released.
https://pypi.python.org/pypi/google-api-python-client/1.4.2

@tmatsuo
Copy link
Contributor Author

tmatsuo commented Sep 24, 2015

@dhermes I haven't seen any issues with the new release for a week, so I removed your original article about cache on App Engine. Thanks!

@dhermes
Copy link
Contributor

dhermes commented Sep 24, 2015

You could have just marked it deprecated. Now I have one less imprint on Google Developers 😞

@tmatsuo
Copy link
Contributor Author

tmatsuo commented Sep 24, 2015

Yeah, in that sense, I initially tried to mark it as deprecated, but tech writers don't like it. You're doing a greater work on gcloud-python, so I thought you're ok, but let me know if you want it back strongly.

@dhermes
Copy link
Contributor

dhermes commented Sep 24, 2015

Ha no worries Takashi! I was just giving you a hard time. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Discussion: discovery doc caching
4 participants
@tmatsuo @dhermes @nathanielmanistaatgoogle @googlebot