Refactor, simplify and modernize GCS sample

Refactor, simplify and modernize GCS sample.

This is intended to address https://github.com/google/google-api-java-client/issues/938.

[yarbrough, 2015-05-05 01:02:05]

Looks pretty good.

https://codereview.appspot.com/236910043/diff/1/storage-cmdline-sample/src/ma...
File
storage-cmdline-sample/src/main/java/com/google/api/services/samples/storage/cmdline/Helpers.java
(right):

https://codereview.appspot.com/236910043/diff/1/storage-cmdline-sample/src/ma...
storage-cmdline-sample/src/main/java/com/google/api/services/samples/storage/cmdline/Helpers.java:26:
* noticeable lag in stream reading, which detracts from upload speed. This class
takes all that
Could be a significant chunk of memory if it's a big value, but for sample code
that's probably fine.

https://codereview.appspot.com/236910043/diff/1/storage-cmdline-sample/src/ma...
File
storage-cmdline-sample/src/main/java/com/google/api/services/samples/storage/cmdline/StorageSample.java
(right):

https://codereview.appspot.com/236910043/diff/1/storage-cmdline-sample/src/ma...
storage-cmdline-sample/src/main/java/com/google/api/services/samples/storage/cmdline/StorageSample.java:124:
} catch (Throwable t) {
I'd prefer you not catch Throwable and just let it kill the JVM.

https://codereview.appspot.com/236910043/diff/1/storage-cmdline-sample/src/ma...
File
storage-cmdline-sample/src/main/java/com/google/api/services/samples/storage/examples/BucketsInsertExample.java
(right):

https://codereview.appspot.com/236910043/diff/1/storage-cmdline-sample/src/ma...
storage-cmdline-sample/src/main/java/com/google/api/services/samples/storage/examples/BucketsInsertExample.java:44:
public static Bucket insertInNamedProject(Storage storage, String project,
Bucket bucket) throws IOException {
"Named" seems unnecessary as part of the name. Why not just "createBucket"?

https://codereview.appspot.com/236910043/diff/1/storage-cmdline-sample/src/ma...
File
storage-cmdline-sample/src/main/java/com/google/api/services/samples/storage/examples/ObjectsDownloadExample.java
(right):

https://codereview.appspot.com/236910043/diff/1/storage-cmdline-sample/src/ma...
storage-cmdline-sample/src/main/java/com/google/api/services/samples/storage/examples/ObjectsDownloadExample.java:51:
public static void downloadRangeToOutputStream(Storage storage, String
bucketName, String objectName,
This doesn't appear to be used. Perhaps include a comment explaining it.

https://codereview.appspot.com/236910043/diff/1/storage-cmdline-sample/src/ma...
File
storage-cmdline-sample/src/main/java/com/google/api/services/samples/storage/examples/ObjectsGetExample.java
(right):

https://codereview.appspot.com/236910043/diff/1/storage-cmdline-sample/src/ma...
storage-cmdline-sample/src/main/java/com/google/api/services/samples/storage/examples/ObjectsGetExample.java:30:
public class ObjectsGetExample {
For clarity's sake, maybe rename this ObjectGetMetadataExample? Or provide a
reference to the file which demonstrates downloading as a comment.

https://codereview.appspot.com/236910043/diff/1/storage-cmdline-sample/src/ma...
File
storage-cmdline-sample/src/main/java/com/google/api/services/samples/storage/examples/ObjectsListExample.java
(right):

https://codereview.appspot.com/236910043/diff/1/storage-cmdline-sample/src/ma...
storage-cmdline-sample/src/main/java/com/google/api/services/samples/storage/examples/ObjectsListExample.java:44:
} while (objects.getNextPageToken() != null);
The Files API example also checks for an empty nextPageToken. Do we need to do
that as well? https://developers.google.com/drive/v2/reference/files/list

https://codereview.appspot.com/236910043/diff/1/storage-cmdline-sample/src/ma...
File
storage-cmdline-sample/src/main/java/com/google/api/services/samples/storage/examples/ObjectsUploadExample.java
(right):

https://codereview.appspot.com/236910043/diff/1/storage-cmdline-sample/src/ma...
storage-cmdline-sample/src/main/java/com/google/api/services/samples/storage/examples/ObjectsUploadExample.java:56:
insertObject.getMediaHttpUploader().setDisableGZipContent(true);
I forgot the reason why we set disableGZipContent. Probably worth including the
reason it as a comment.

[wonderfly, 2015-05-05 01:44:10]

LGTM.

Just a few nits.

https://codereview.appspot.com/236910043/diff/1/storage-cmdline-sample/src/ma...
File
storage-cmdline-sample/src/main/java/com/google/api/services/samples/storage/cmdline/StorageSample.java
(right):

https://codereview.appspot.com/236910043/diff/1/storage-cmdline-sample/src/ma...
storage-cmdline-sample/src/main/java/com/google/api/services/samples/storage/cmdline/StorageSample.java:25:
import com.google.api.services.samples.storage.examples.BucketsGetExample;
Nitpick:

If you passed --git_no_find_copies=False to upload.py, it'd detect that you
based off this file and created those new ones, thus preserving the history and
showing a better diff.

See if you can get it when you upload your next patchset (you'd have to run it
at least once anyway to address comments). If not, consider doing it next time.

https://codereview.appspot.com/236910043/diff/1/storage-cmdline-sample/src/ma...
storage-cmdline-sample/src/main/java/com/google/api/services/samples/storage/cmdline/StorageSample.java:57:
JsonFactory JSON_FACTORY = JacksonFactory.getDefaultInstance();
CAPITALIZED_WORDS are usually constants. Consider lower case here?

https://codereview.appspot.com/236910043/diff/1/storage-cmdline-sample/src/ma...
File
storage-cmdline-sample/src/main/java/com/google/api/services/samples/storage/examples/BucketsInsertExample.java
(right):

https://codereview.appspot.com/236910043/diff/1/storage-cmdline-sample/src/ma...
storage-cmdline-sample/src/main/java/com/google/api/services/samples/storage/examples/BucketsInsertExample.java:50:
if (error.getCode() == HTTP_CONFLICT
I'd check if error is null. It's possible the server would return a non-JSON
error response, e.g., many clients have seen 401 responses with empty payload,
in which case e.getDetails() returns null.

[nherring, 2015-05-05 15:09:00]

Addressed review commentary, and added a crc32c validation step. PTAL.

https://codereview.appspot.com/236910043/diff/1/storage-cmdline-sample/src/ma...
File
storage-cmdline-sample/src/main/java/com/google/api/services/samples/storage/cmdline/Helpers.java
(right):

https://codereview.appspot.com/236910043/diff/1/storage-cmdline-sample/src/ma...
storage-cmdline-sample/src/main/java/com/google/api/services/samples/storage/cmdline/Helpers.java:26:
* noticeable lag in stream reading, which detracts from upload speed. This class
takes all that
On 2015/05/05 01:02:04, yarbrough wrote:
> Could be a significant chunk of memory if it's a big value, but for sample
code
> that's probably fine.

It's controllable by the consumer. This sample uses a single random block of
1024 bytes and reads out of it circularly to fill the size.

https://codereview.appspot.com/236910043/diff/1/storage-cmdline-sample/src/ma...
File
storage-cmdline-sample/src/main/java/com/google/api/services/samples/storage/cmdline/StorageSample.java
(right):

https://codereview.appspot.com/236910043/diff/1/storage-cmdline-sample/src/ma...
storage-cmdline-sample/src/main/java/com/google/api/services/samples/storage/cmdline/StorageSample.java:25:
import com.google.api.services.samples.storage.examples.BucketsGetExample;
On 2015/05/05 01:44:10, wonderfly wrote:
> Nitpick:
> 
> If you passed --git_no_find_copies=False to upload.py, it'd detect that you
> based off this file and created those new ones, thus preserving the history
and
> showing a better diff.
> 
> See if you can get it when you upload your next patchset (you'd have to run it
> at least once anyway to address comments). If not, consider doing it next
time.
> 

Patch set #2 uses --git_no_find_copies. It complains if you give it an argument,
and it doesn't recognize --no_git_no_find_copies.

https://codereview.appspot.com/236910043/diff/1/storage-cmdline-sample/src/ma...
storage-cmdline-sample/src/main/java/com/google/api/services/samples/storage/cmdline/StorageSample.java:57:
JsonFactory JSON_FACTORY = JacksonFactory.getDefaultInstance();
On 2015/05/05 01:44:10, wonderfly wrote:
> CAPITALIZED_WORDS are usually constants. Consider lower case here?

Done.

https://codereview.appspot.com/236910043/diff/1/storage-cmdline-sample/src/ma...
storage-cmdline-sample/src/main/java/com/google/api/services/samples/storage/cmdline/StorageSample.java:124:
} catch (Throwable t) {
On 2015/05/05 01:02:04, yarbrough wrote:
> I'd prefer you not catch Throwable and just let it kill the JVM.

I could do that, but it will not show the stack trace. Is that preferable? I
could always rethrow the throwable after doing so, and there'd be the duplicate
line where the message is printed a second time.

https://codereview.appspot.com/236910043/diff/1/storage-cmdline-sample/src/ma...
File
storage-cmdline-sample/src/main/java/com/google/api/services/samples/storage/examples/BucketsInsertExample.java
(right):

https://codereview.appspot.com/236910043/diff/1/storage-cmdline-sample/src/ma...
storage-cmdline-sample/src/main/java/com/google/api/services/samples/storage/examples/BucketsInsertExample.java:44:
public static Bucket insertInNamedProject(Storage storage, String project,
Bucket bucket) throws IOException {
On 2015/05/05 01:02:04, yarbrough wrote:
> "Named" seems unnecessary as part of the name.

Fair.

> Why not just "createBucket"?

Originally because JSON's language is all around insert rather than create. If
you think using create here for the example would help illustrate, then let's do
that.

https://codereview.appspot.com/236910043/diff/1/storage-cmdline-sample/src/ma...
storage-cmdline-sample/src/main/java/com/google/api/services/samples/storage/examples/BucketsInsertExample.java:50:
if (error.getCode() == HTTP_CONFLICT
On 2015/05/05 01:44:10, wonderfly wrote:
> I'd check if error is null. It's possible the server would return a non-JSON
> error response, e.g., many clients have seen 401 responses with empty payload,
> in which case e.getDetails() returns null.

Done.

https://codereview.appspot.com/236910043/diff/1/storage-cmdline-sample/src/ma...
File
storage-cmdline-sample/src/main/java/com/google/api/services/samples/storage/examples/ObjectsDownloadExample.java
(right):

https://codereview.appspot.com/236910043/diff/1/storage-cmdline-sample/src/ma...
storage-cmdline-sample/src/main/java/com/google/api/services/samples/storage/examples/ObjectsDownloadExample.java:51:
public static void downloadRangeToOutputStream(Storage storage, String
bucketName, String objectName,
On 2015/05/05 01:02:05, yarbrough wrote:
> This doesn't appear to be used. Perhaps include a comment explaining it.

Done.

https://codereview.appspot.com/236910043/diff/1/storage-cmdline-sample/src/ma...
File
storage-cmdline-sample/src/main/java/com/google/api/services/samples/storage/examples/ObjectsGetExample.java
(right):

https://codereview.appspot.com/236910043/diff/1/storage-cmdline-sample/src/ma...
storage-cmdline-sample/src/main/java/com/google/api/services/samples/storage/examples/ObjectsGetExample.java:30:
public class ObjectsGetExample {
On 2015/05/05 01:02:05, yarbrough wrote:
> For clarity's sake, maybe rename this ObjectGetMetadataExample? Or provide a
> reference to the file which demonstrates downloading as a comment.

Done.

https://codereview.appspot.com/236910043/diff/1/storage-cmdline-sample/src/ma...
File
storage-cmdline-sample/src/main/java/com/google/api/services/samples/storage/examples/ObjectsListExample.java
(right):

https://codereview.appspot.com/236910043/diff/1/storage-cmdline-sample/src/ma...
storage-cmdline-sample/src/main/java/com/google/api/services/samples/storage/examples/ObjectsListExample.java:44:
} while (objects.getNextPageToken() != null);
On 2015/05/05 01:02:05, yarbrough wrote:
> The Files API example also checks for an empty nextPageToken. Do we need to do
> that as well? https://developers.google.com/drive/v2/reference/files/list

Do we ever return an empty but extant page token in the API? I don't think we
do.

https://codereview.appspot.com/236910043/diff/1/storage-cmdline-sample/src/ma...
File
storage-cmdline-sample/src/main/java/com/google/api/services/samples/storage/examples/ObjectsUploadExample.java
(right):

https://codereview.appspot.com/236910043/diff/1/storage-cmdline-sample/src/ma...
storage-cmdline-sample/src/main/java/com/google/api/services/samples/storage/examples/ObjectsUploadExample.java:56:
insertObject.getMediaHttpUploader().setDisableGZipContent(true);
On 2015/05/05 01:02:05, yarbrough wrote:
> I forgot the reason why we set disableGZipContent. Probably worth including
the
> reason it as a comment.

Done.

https://codereview.appspot.com/236910043/diff/20001/storage-cmdline-sample/sr...
File
storage-cmdline-sample/src/main/java/com/google/api/services/samples/storage/cmdline/StorageSample.java
(right):

https://codereview.appspot.com/236910043/diff/20001/storage-cmdline-sample/sr...
storage-cmdline-sample/src/main/java/com/google/api/services/samples/storage/cmdline/StorageSample.java:110:
HashingOutputStream crc32cHashingOutputStream = new
HashingOutputStream(Hashing.crc32c(), md5DigestOutputStream);
I added a crc32c calculator here, and validation below. PTAL.

[marcgel, 2015-05-05 16:47:34]

Update instructions.html too?

[nherring, 2015-05-05 19:27:37]

On 2015/05/05 16:47:34, marcgel wrote:
> Update instructions.html too?

Done.

[marcgel, 2015-05-05 20:05:37]

On 2015/05/05 19:27:37, nherring wrote:
> On 2015/05/05 16:47:34, marcgel wrote:
> > Update instructions.html too?
> 
> Done.

Thanks, instructions LGTM.

[wonderfly, 2015-05-05 20:39:26]

LGTM.

I'll update the instructions for all other samples when I get time.

[yarbrough, 2015-05-06 21:30:17]

LGTM

[wonderfly, 2015-05-08 23:55:29]

The patch is committed:
https://github.com/google/google-api-java-client-samples/commit/af27369683f2f...

You can close this issue now.