Added examples for using the JSON API for file transfers.

Added examples for using the JSON API for file transfers.

[nherring, 2012-08-15 02:34:46]

http://codereview.appspot.com/6458129/diff/1/file-transfer-json/chunked_downl... File file-transfer-json/chunked_download.py (right): http://codereview.appspot.com/6458129/diff/1/file-transfer-json/chunked_downl... file-transfer-json/chunked_download.py:48: sleeptime = random.random() + (2**progressless_iters) Generally exponential backoff uses a random sleep time between 0 and 2**progressless_iters - 1. The Java API client uses a slightly different mechanism: http://code.google.com/p/google-http-java-client/source/browse/google-http-cl... This will work, it just means that many such clients hitting the same error will retry in lockstep +/- 0.5s, and I'm not sure the random adds enough slop to not have it repeatedly overwhelm the service in waves if they'd been all overwhelming it previously. http://codereview.appspot.com/6458129/diff/1/file-transfer-json/chunked_downl... file-transfer-json/chunked_download.py:72: HandleProgresslessIter(error, progressless_iters, num_retries) Should we reset progressless_iters after a success? http://codereview.appspot.com/6458129/diff/1/file-transfer-json/chunked_uploa... File file-transfer-json/chunked_upload.py (right): http://codereview.appspot.com/6458129/diff/1/file-transfer-json/chunked_uploa... file-transfer-json/chunked_upload.py:11: """ We should add the Apache 2 boilerplate to all of these.

[yovadia, 2012-08-15 15:23:01]

Thanks; PTAL. http://codereview.appspot.com/6458129/diff/1/file-transfer-json/chunked_downl... File file-transfer-json/chunked_download.py (right): http://codereview.appspot.com/6458129/diff/1/file-transfer-json/chunked_downl... file-transfer-json/chunked_download.py:48: sleeptime = random.random() + (2**progressless_iters) On 2012/08/15 02:34:46, nherring wrote: > Generally exponential backoff uses a random sleep time between 0 and > 2**progressless_iters - 1. The Java API client uses a slightly different > mechanism: > http://code.google.com/p/google-http-java-client/source/browse/google-http-cl... > This will work, it just means that many such clients hitting the same error will > retry in lockstep +/- 0.5s, and I'm not sure the random adds enough slop to not > have it repeatedly overwhelm the service in waves if they'd been all > overwhelming it previously. Done. http://codereview.appspot.com/6458129/diff/1/file-transfer-json/chunked_downl... file-transfer-json/chunked_download.py:72: HandleProgresslessIter(error, progressless_iters, num_retries) On 2012/08/15 02:34:46, nherring wrote: > Should we reset progressless_iters after a success? Done. http://codereview.appspot.com/6458129/diff/1/file-transfer-json/chunked_uploa... File file-transfer-json/chunked_upload.py (right): http://codereview.appspot.com/6458129/diff/1/file-transfer-json/chunked_uploa... file-transfer-json/chunked_upload.py:11: """ On 2012/08/15 02:34:46, nherring wrote: > We should add the Apache 2 boilerplate to all of these. What do you mean by Apache2 boilerplate?

[nherring, 2012-08-15 16:27:08]

http://codereview.appspot.com/6458129/diff/1/file-transfer-json/chunked_uploa... File file-transfer-json/chunked_upload.py (right): http://codereview.appspot.com/6458129/diff/1/file-transfer-json/chunked_uploa... file-transfer-json/chunked_upload.py:11: """ On 2012/08/15 15:23:02, yovadia wrote: > On 2012/08/15 02:34:46, nherring wrote: > > We should add the Apache 2 boilerplate to all of these. > > What do you mean by Apache2 boilerplate? lines 3-13 of http://code.google.com/p/google-api-python-client/source/browse/apiclient/htt...

[yovadia, 2012-08-15 16:32:46]

Thanks; PTAL. http://codereview.appspot.com/6458129/diff/1/file-transfer-json/chunked_uploa... File file-transfer-json/chunked_upload.py (right): http://codereview.appspot.com/6458129/diff/1/file-transfer-json/chunked_uploa... file-transfer-json/chunked_upload.py:11: """ On 2012/08/15 16:27:08, nherring wrote: > On 2012/08/15 15:23:02, yovadia wrote: > > On 2012/08/15 02:34:46, nherring wrote: > > > We should add the Apache 2 boilerplate to all of these. > > > > What do you mean by Apache2 boilerplate? > > lines 3-13 of > http://code.google.com/p/google-api-python-client/source/browse/apiclient/htt... > Done.

[nherring, 2012-08-15 17:26:29]

LGTM

[marccohen, 2012-08-21 00:30:47]

http://codereview.appspot.com/6458129/diff/10001/file-transfer-json/chunked_d... File file-transfer-json/chunked_download.py (right): http://codereview.appspot.com/6458129/diff/10001/file-transfer-json/chunked_d... file-transfer-json/chunked_download.py:32: import httplib2 nit picky but probably cleaner to keep imports together and before 'from...import's. http://codereview.appspot.com/6458129/diff/10001/file-transfer-json/chunked_d... file-transfer-json/chunked_download.py:41: client_id='', suggest filling these values in with a placeholder string, e.g. CLIENT_ID, CLIENT_SECRET, to make them stand out more. Also, if a user neglects to fix them, they'll stand out in error messages more prominently than an empty string. http://codereview.appspot.com/6458129/diff/10001/file-transfer-json/chunked_d... file-transfer-json/chunked_download.py:52: CHUNKSIZE = 2 * 1024 * 1024 Should this be an optional command line arg? http://codereview.appspot.com/6458129/diff/10001/file-transfer-json/chunked_d... file-transfer-json/chunked_download.py:69: while done is False: 'while not done' seems simpler and reads like english http://codereview.appspot.com/6458129/diff/10001/file-transfer-json/chunked_d... file-transfer-json/chunked_download.py:74: print 'Download %d%%.' % int(progress.progress() * 100) Does this print a new line every time? Is it possible/worth overwriting a single line by emitting backspaces and suppressing the trailing newline? http://codereview.appspot.com/6458129/diff/10001/file-transfer-json/chunked_d... file-transfer-json/chunked_download.py:92: bucket_name, object_name = argv[1].split('/', 1) Should we require the gs:// prefix, just for consistency with gsutil? I can see arguing this both ways so I'm happy to go with your judgement call here. http://codereview.appspot.com/6458129/diff/10001/file-transfer-json/chunked_d... file-transfer-json/chunked_download.py:111: bucket_name, Printing the object name before the bucket name feels in reverse order to most other UI elements where we display both values. http://codereview.appspot.com/6458129/diff/10001/file-transfer-json/chunked_u... File file-transfer-json/chunked_upload.py (right): http://codereview.appspot.com/6458129/diff/10001/file-transfer-json/chunked_u... file-transfer-json/chunked_upload.py:25: global comment: seems like a lot of dup code here. Could some of the duplication be eliminated by defining a shared module? Even better, should this be one script and one of the args indicates the copy direction? Even if you don't do anything to consolidate code, several comments for the other file apply here as well.

[jcgregorio_google, 2012-08-21 13:05:14]

http://codereview.appspot.com/6458129/diff/10001/file-transfer-json/chunked_d... File file-transfer-json/chunked_download.py (right): http://codereview.appspot.com/6458129/diff/10001/file-transfer-json/chunked_d... file-transfer-json/chunked_download.py:93: filename = argv[2] Use gflags for processing command-line args, see: http://code.google.com/p/google-api-python-client/source/browse/samples/plus/... http://codereview.appspot.com/6458129/diff/10001/file-transfer-json/chunked_u... File file-transfer-json/chunked_upload.py (right): http://codereview.appspot.com/6458129/diff/10001/file-transfer-json/chunked_u... file-transfer-json/chunked_upload.py:38: FLOW = OAuth2WebServerFlow( Use the client_secrets.json file format and supporting methods, see the Google+ sample as an example: http://code.google.com/p/google-api-python-client/source/browse/samples/plus/... http://codereview.appspot.com/6458129/diff/10001/file-transfer-json/chunked_u... file-transfer-json/chunked_upload.py:70: def Upload(request, num_retries=5): There is already an exponential back-off class in the threadqueue sample: http://code.google.com/p/google-api-python-client/source/browse/samples/threa... Can we combine both of these together into a utility class that goes into apiclient.http? Something that looks like: class ExponentialBackoff(): def __init__(self, request, http=None, <other params like max iters>): """request is any object that has an .execute(http) method.""" ... def execute(http=http): """Does the actual work""" ...

[yovadia, 2012-08-21 21:08:31]

Thanks for the review; PTAL. Also, I merged in the repo changes since checking in the GAE bucket lister, so it's showing up in the review. You need not bother with anything in that directory. http://codereview.appspot.com/6458129/diff/10001/file-transfer-json/chunked_d... File file-transfer-json/chunked_download.py (right): http://codereview.appspot.com/6458129/diff/10001/file-transfer-json/chunked_d... file-transfer-json/chunked_download.py:32: import httplib2 On 2012/08/21 00:30:47, marccohen wrote: > nit picky but probably cleaner to keep imports together and before > 'from...import's. Done. http://codereview.appspot.com/6458129/diff/10001/file-transfer-json/chunked_d... file-transfer-json/chunked_download.py:41: client_id='', On 2012/08/21 00:30:47, marccohen wrote: > suggest filling these values in with a placeholder string, e.g. CLIENT_ID, > CLIENT_SECRET, to make them stand out more. Also, if a user neglects to fix > them, they'll stand out in error messages more prominently than an empty string. Done. http://codereview.appspot.com/6458129/diff/10001/file-transfer-json/chunked_d... file-transfer-json/chunked_download.py:52: CHUNKSIZE = 2 * 1024 * 1024 On 2012/08/21 00:30:47, marccohen wrote: > Should this be an optional command line arg? Since this is meant more as a code sample than as the primary mechanism for customers to upload/download objects, I was inclined to keep things simple. Let me know if you'd still prefer to have a chunksize argument. http://codereview.appspot.com/6458129/diff/10001/file-transfer-json/chunked_d... file-transfer-json/chunked_download.py:69: while done is False: On 2012/08/21 00:30:47, marccohen wrote: > 'while not done' seems simpler and reads like english Done. http://codereview.appspot.com/6458129/diff/10001/file-transfer-json/chunked_d... file-transfer-json/chunked_download.py:74: print 'Download %d%%.' % int(progress.progress() * 100) On 2012/08/21 00:30:47, marccohen wrote: > Does this print a new line every time? Is it possible/worth overwriting a single > line by emitting backspaces and suppressing the trailing newline? As with the chunksize parameter, I was hesitant to add needless complexity to a simple example. I've added the feature; let me know if you think it's worth keeping. http://codereview.appspot.com/6458129/diff/10001/file-transfer-json/chunked_d... file-transfer-json/chunked_download.py:92: bucket_name, object_name = argv[1].split('/', 1) On 2012/08/21 00:30:47, marccohen wrote: > Should we require the gs:// prefix, just for consistency with gsutil? I can see > arguing this both ways so I'm happy to go with your judgement call here. I ended up using this since it also distinguishes between up/downloads. Let me know if you think it's simple enough to keep. http://codereview.appspot.com/6458129/diff/10001/file-transfer-json/chunked_d... file-transfer-json/chunked_download.py:93: filename = argv[2] On 2012/08/21 13:05:14, jcgregorio_google wrote: > Use gflags for processing command-line args, see: > > http://code.google.com/p/google-api-python-client/source/browse/samples/plus/... Since this is meant to be a simple introductory sample to using the API client with GCS, I'd rather not clutter it with too many features and libraries. http://codereview.appspot.com/6458129/diff/10001/file-transfer-json/chunked_d... file-transfer-json/chunked_download.py:111: bucket_name, On 2012/08/21 00:30:47, marccohen wrote: > Printing the object name before the bucket name feels in reverse order to most > other UI elements where we display both values. Done. http://codereview.appspot.com/6458129/diff/10001/file-transfer-json/chunked_u... File file-transfer-json/chunked_upload.py (right): http://codereview.appspot.com/6458129/diff/10001/file-transfer-json/chunked_u... file-transfer-json/chunked_upload.py:25: On 2012/08/21 00:30:47, marccohen wrote: > global comment: seems like a lot of dup code here. Could some of the duplication > be eliminated by defining a shared module? Even better, should this be one > script and one of the args indicates the copy direction? > > Even if you don't do anything to consolidate code, several comments for the > other file apply here as well. Done. http://codereview.appspot.com/6458129/diff/10001/file-transfer-json/chunked_u... file-transfer-json/chunked_upload.py:38: FLOW = OAuth2WebServerFlow( On 2012/08/21 13:05:14, jcgregorio_google wrote: > Use the client_secrets.json file format and supporting methods, see the Google+ > sample as an example: > > http://code.google.com/p/google-api-python-client/source/browse/samples/plus/... It may be worth having a wiki page describing this file format. Also, why is this preferable to simply having a client ID/secret in the source? http://codereview.appspot.com/6458129/diff/10001/file-transfer-json/chunked_u... file-transfer-json/chunked_upload.py:70: def Upload(request, num_retries=5): On 2012/08/21 13:05:14, jcgregorio_google wrote: > There is already an exponential back-off class in the threadqueue sample: > > http://code.google.com/p/google-api-python-client/source/browse/samples/threa... > > Can we combine both of these together into a utility class that goes into > apiclient.http? Something that looks like: > > class ExponentialBackoff(): > def __init__(self, request, http=None, <other params like max iters>): > """request is any object that has an .execute(http) method.""" > ... > > def execute(http=http): > """Does the actual work""" > ... I'd actually be interested in having this kind of behavior built into something like HttpRequest with exponential backoff and callback arguments to handle failure and progress printing, but for the purposes of this sample, it should be sufficient to inline some simple backoff behavior. If/when we get a sufficiently simple-to-use, built-in exponential backoff handler handler ready, we can add it to the sample. Let me know if that sounds reasonable.

[jcgregorio_google, 2012-08-21 23:41:20]

http://codereview.appspot.com/6458129/diff/10001/file-transfer-json/chunked_u... File file-transfer-json/chunked_upload.py (right): http://codereview.appspot.com/6458129/diff/10001/file-transfer-json/chunked_u... file-transfer-json/chunked_upload.py:38: FLOW = OAuth2WebServerFlow( On 2012/08/21 21:08:31, yovadia wrote: > On 2012/08/21 13:05:14, jcgregorio_google wrote: > > Use the client_secrets.json file format and supporting methods, see the > Google+ > > sample as an example: > > > > > http://code.google.com/p/google-api-python-client/source/browse/samples/plus/... > > It may be worth having a wiki page describing this file format. http://code.google.com/p/google-api-python-client/wiki/ClientSecrets > > Also, why is this preferable to simply having a client ID/secret in the source? Client id and client secrets shouldn't go in code, per the security team.

[marccohen, 2012-08-23 22:35:18]

LGTM, modulo one typo. I like what you've done to combined upload and download flows into one script and eliminate dup code. Nice job! http://codereview.appspot.com/6458129/diff/6003/file-transfer-json/chunked_tr... File file-transfer-json/chunked_transfer.py (right): http://codereview.appspot.com/6458129/diff/6003/file-transfer-json/chunked_tr... file-transfer-json/chunked_transfer.py:49: # File where we will store authentication credentials after aquiring them. typo: acquiring

[jcgregorio_google, 2012-08-23 23:06:20]

http://codereview.appspot.com/6458129/diff/6003/file-transfer-json/chunked_tr... File file-transfer-json/chunked_transfer.py (right): http://codereview.appspot.com/6458129/diff/6003/file-transfer-json/chunked_tr... file-transfer-json/chunked_transfer.py:91: def GetAuthenticatedService(scope): get_authenticated_service to keep it consistent with the rest of the library which follows PEP8 naming conventions. Applies to all method names.

[yovadia, 2012-08-24 00:24:00]

Thanks PTAL. http://codereview.appspot.com/6458129/diff/6003/file-transfer-json/chunked_tr... File file-transfer-json/chunked_transfer.py (right): http://codereview.appspot.com/6458129/diff/6003/file-transfer-json/chunked_tr... file-transfer-json/chunked_transfer.py:49: # File where we will store authentication credentials after aquiring them. On 2012/08/23 22:35:18, marccohen wrote: > typo: acquiring Done. http://codereview.appspot.com/6458129/diff/6003/file-transfer-json/chunked_tr... file-transfer-json/chunked_transfer.py:91: def GetAuthenticatedService(scope): On 2012/08/23 23:06:20, jcgregorio_google wrote: > get_authenticated_service to keep it consistent with the rest of the library > which follows PEP8 naming conventions. Applies to all method names. Done.

[jcgregorio_google, 2012-08-24 00:35:16]

http://codereview.appspot.com/6458129/diff/15003/file-transfer-json/README.txt File file-transfer-json/README.txt (right): http://codereview.appspot.com/6458129/diff/15003/file-transfer-json/README.tx... file-transfer-json/README.txt:1: File Transfer with Google Cloud Storage and the Google APIs Python Client Library Looks good, but please follow the style guide for README files so your samples get properly categorized on the wiki samples page: http://code.google.com/p/google-api-python-client/wiki/BecomingAContributor#R...

[yovadia, 2012-08-24 00:43:57]

Fixed the README; PTAL. http://codereview.appspot.com/6458129/diff/15003/file-transfer-json/README.txt File file-transfer-json/README.txt (right): http://codereview.appspot.com/6458129/diff/15003/file-transfer-json/README.tx... file-transfer-json/README.txt:1: File Transfer with Google Cloud Storage and the Google APIs Python Client Library On 2012/08/24 00:35:17, jcgregorio_google wrote: > Looks good, but please follow the style guide for README files so your samples > get properly categorized on the wiki samples page: > > > http://code.google.com/p/google-api-python-client/wiki/BecomingAContributor#R... Done.

[jcgregorio_google, 2012-08-24 01:08:01]

http://codereview.appspot.com/6458129/diff/16003/file-transfer-json/README File file-transfer-json/README (right): http://codereview.appspot.com/6458129/diff/16003/file-transfer-json/README#ne... file-transfer-json/README:5: keywords: storage, json, upload, download, media Sorry, I forgot to point out that the list of valid keywords is here: http://code.google.com/p/google-api-python-client/source/browse/samples-index... I updated the wiki page with that information. I do think 'media' is a good category for samples, could you add that as a KEYWORD entry?

[yovadia, 2012-08-24 01:20:43]

Thanks; PTAL. http://codereview.appspot.com/6458129/diff/16003/file-transfer-json/README File file-transfer-json/README (right): http://codereview.appspot.com/6458129/diff/16003/file-transfer-json/README#ne... file-transfer-json/README:5: keywords: storage, json, upload, download, media On 2012/08/24 01:08:01, jcgregorio_google wrote: > Sorry, I forgot to point out that the list of valid keywords is here: > > > http://code.google.com/p/google-api-python-client/source/browse/samples-index... > > I updated the wiki page with that information. > > I do think 'media' is a good category for samples, could you add that as a > KEYWORD entry? Done.

[jcgregorio_google, 2012-08-24 01:21:21]

LGTM On 2012/08/24 01:20:43, yovadia wrote: > Thanks; PTAL. > > http://codereview.appspot.com/6458129/diff/16003/file-transfer-json/README > File file-transfer-json/README (right): > > http://codereview.appspot.com/6458129/diff/16003/file-transfer-json/README#ne... > file-transfer-json/README:5: keywords: storage, json, upload, download, media > On 2012/08/24 01:08:01, jcgregorio_google wrote: > > Sorry, I forgot to point out that the list of valid keywords is here: > > > > > > > http://code.google.com/p/google-api-python-client/source/browse/samples-index... > > > > I updated the wiki page with that information. > > > > I do think 'media' is a good category for samples, could you add that as a > > KEYWORD entry? > > Done.

[yovadia, 2012-08-24 01:27:05]

Submitted to: http://code.google.com/p/google-cloud-platform-samples/source/browse?repo=sto... On 2012/08/24 01:21:21, jcgregorio_google wrote: > LGTM > > On 2012/08/24 01:20:43, yovadia wrote: > > Thanks; PTAL. > > > > http://codereview.appspot.com/6458129/diff/16003/file-transfer-json/README > > File file-transfer-json/README (right): > > > > > http://codereview.appspot.com/6458129/diff/16003/file-transfer-json/README#ne... > > file-transfer-json/README:5: keywords: storage, json, upload, download, media > > On 2012/08/24 01:08:01, jcgregorio_google wrote: > > > Sorry, I forgot to point out that the list of valid keywords is here: > > > > > > > > > > > > http://code.google.com/p/google-api-python-client/source/browse/samples-index... > > > > > > I updated the wiki page with that information. > > > > > > I do think 'media' is a good category for samples, could you add that as a > > > KEYWORD entry? > > > > Done.