Issue 5450097: Adding Resumable Media Upload support for Google APIs (Closed)
Patch Set 1 #
Total comments: 22
Patch Set 2 : Addressing review comments #
Total comments: 107
Patch Set 3 : Modified GoogleHeaders and MediaHttpUploader due to 5482047. #Patch Set 4 : Minor fixes #Patch Set 5 : Addressing review comments #Patch Set 6 : Minor fixes #
Total comments: 24
Patch Set 7 : Addressing code review comments #
Total comments: 15
Patch Set 8 : Addressing code review comments #Patch Set 9 : Minor fixes #Patch Set 10 : Minor fixes #
Total comments: 6
Patch Set 11 : Addressing code review comments #Patch Set 12 : Minor fixes #
Total comments: 2
Patch Set 13 : Merging #
Total comments: 2
Patch Set 14 : Addressing code review comments #Patch Set 15 : Merging #
Total comments: 14
Patch Set 16 : Addressing code review comments #Patch Set 17 : Minor fixes #
Total comments: 2
Patch Set 18 : Addressing code review comments #Patch Set 19 : Call AbstractInputStreamContent.setCloseInputStream(false) #Patch Set 20 : Calling setCloseInputStream(false) in the right place #MessagesTotal messages: 28
first round of comments... http://codereview.appspot.com/5450097/diff/1/google-api-client/src/main/java/... File google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java (right): http://codereview.appspot.com/5450097/diff/1/google-api-client/src/main/java/... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:189: request.setContent(ByteArrayContent.fromString(mediaContent.getType(), " ")); This is wrong. There should be no "Content-Type" header. Instead, the specification at: http://code.google.com/apis/gdata/docs/resumable_upload.html says that you should set the X-Upload-Content-Type and X-Upload-Content-Length on the initial POST request, regardless of whether there is metadata http://codereview.appspot.com/5450097/diff/1/google-api-client/src/main/java/... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:230: request.setContent(ByteArrayContent.fromString(mediaContent.getType(), " ")); this line should not be needed because there should be no Content-Type header according to the specification at: http://code.google.com/apis/gdata/docs/resumable_upload.html and also because HttpRequest already sets the content to " " if there is no content. http://codereview.appspot.com/5450097/diff/1/google-api-client/src/main/java/... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:244: contentInputStream.skip(bytesWritten); Interesting bug report from running 'mvn findbugs:gui': " Method ignores results of InputStream.skip() This method ignores the return value of java.io.InputStream.skip() which can skip multiple bytes. If the return value is not checked, the caller will not be able to correctly handle the case where fewer bytes were skipped than the caller requested. This is a particularly insidious kind of bug, because in many programs, skips from input streams usually do skip the full amount of data requested, causing the program to fail only sporadically. With Buffered streams, however, skip() will only skip data in the buffer, and will routinely fail to skip the requested number of bytes. " http://codereview.appspot.com/5450097/diff/1/google-api-client/src/main/java/... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:260: if (rangeHeader == null || rangeHeader.indexOf('-') == -1) { we already computed the index of '-', so you should be able to write something as simple as: int dash = rangeHeader.indexOf('-'); long firstByteIndex = Long.parseLong(rangeHeader.substring(0, dash)); long lastByteIndex = Long.parseLong(rangeHeader.substring(dash + 1)); http://codereview.appspot.com/5450097/diff/1/google-api-client/src/main/java/... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:261: // No valid range header, start from the beginning of the file. are we really going to encounter an invalid range header? seems unnecessary to me to be so paranoid that the Google server doesn't behave as specified. http://codereview.appspot.com/5450097/diff/1/google-api-client/src/main/java/... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:378: public double getProgress() { [optional] do we really need getProgress()? isn't it sufficient to provide getNumBytesUploaded() and getMediaContentLength()? http://codereview.appspot.com/5450097/diff/1/google-api-client/src/main/java/... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:381: return uploadState.equals(UploadState.COMPLETE) ? 1 : 0; It seems really odd that getNumBytesUploaded() when UploadState.COMPLETE. Should it return mediaContent.getLength() at that point? http://codereview.appspot.com/5450097/diff/1/google-api-client/src/main/java/... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:383: return (double) bytesUploaded / mediaContent.getLength(); please cache mediaContent.getLength() just in case it is expensive to compute, and provide a getter method for it http://codereview.appspot.com/5450097/diff/1/google-api-client/src/main/java/... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:389: private class NotificationTask extends TimerTask { Given that there is an overhead to Java threads I'd prefer not to start threads. Also, App Engine doesn't support threads, so we would lose progress listener support for App Engine. Why don't we just only call listener.progressChanged when bytesUpload changes?
Sign in to reply to this message.
http://codereview.appspot.com/5450097/diff/1/google-api-client/src/main/java/... File google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java (right): http://codereview.appspot.com/5450097/diff/1/google-api-client/src/main/java/... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:117: public MediaHttpUploader(HttpContent metadata, InputStreamContent mediaContent, AbstractInputStreamContent is a better choice than InputStreamContent because then we can also support FileContent and ByteArrayContent should we make getInputStream() public? http://codereview.appspot.com/5450097/diff/1/google-api-client/src/main/java/... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:260: if (rangeHeader == null || rangeHeader.indexOf('-') == -1) { On 2011/12/05 22:00:09, yanivi wrote: > we already computed the index of '-', so you should be able to write something > as simple as: > int dash = rangeHeader.indexOf('-'); > long firstByteIndex = Long.parseLong(rangeHeader.substring(0, dash)); > long lastByteIndex = Long.parseLong(rangeHeader.substring(dash + 1)); > doesn't this whole thing just reduce to something like: return Long.parseLong(rangeHeader.substring(rangeHeader.indexOf('-') + 1));
Sign in to reply to this message.
Supporting changes are in: http://codereview.appspot.com/5482047/ http://codereview.appspot.com/5450097/diff/1/google-api-client/src/main/java/... File google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java (right): http://codereview.appspot.com/5450097/diff/1/google-api-client/src/main/java/... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:117: public MediaHttpUploader(HttpContent metadata, InputStreamContent mediaContent, On 2011/12/05 22:12:57, yanivi wrote: > AbstractInputStreamContent is a better choice than InputStreamContent because > then we can also support FileContent and ByteArrayContent > > should we make getInputStream() public? Done. http://codereview.appspot.com/5450097/diff/1/google-api-client/src/main/java/... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:189: request.setContent(ByteArrayContent.fromString(mediaContent.getType(), " ")); On 2011/12/05 22:00:09, yanivi wrote: > This is wrong. There should be no "Content-Type" header. Instead, the > specification at: > > http://code.google.com/apis/gdata/docs/resumable_upload.html > > says that you should set the X-Upload-Content-Type and X-Upload-Content-Length > on the initial POST request, regardless of whether there is metadata Yes I am not sure how I missed this. Maybe I thought that it was GData specific but it is not, it is the content type and content length of the media to be uploaded. http://codereview.appspot.com/5450097/diff/1/google-api-client/src/main/java/... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:230: request.setContent(ByteArrayContent.fromString(mediaContent.getType(), " ")); On 2011/12/05 22:00:09, yanivi wrote: > this line should not be needed because there should be no Content-Type header > according to the specification at: > > http://code.google.com/apis/gdata/docs/resumable_upload.html > > and also because HttpRequest already sets the content to " " if there is no > content. Done. http://codereview.appspot.com/5450097/diff/1/google-api-client/src/main/java/... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:244: contentInputStream.skip(bytesWritten); On 2011/12/05 22:00:09, yanivi wrote: > Interesting bug report from running 'mvn findbugs:gui': > > " > Method ignores results of InputStream.skip() > > This method ignores the return value of java.io.InputStream.skip() which can > skip multiple bytes. If the return value is not checked, the caller will not be > able to correctly handle the case where fewer bytes were skipped than the caller > requested. This is a particularly insidious kind of bug, because in many > programs, skips from input streams usually do skip the full amount of data > requested, causing the program to fail only sporadically. With Buffered streams, > however, skip() will only skip data in the buffer, and will routinely fail to > skip the requested number of bytes. > " It is strange that I do not see this msg when I run mvn findbugs:gui. Also not sure how to handle the bug you see, I will be happy to implement any recommendations you have. http://codereview.appspot.com/5450097/diff/1/google-api-client/src/main/java/... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:260: if (rangeHeader == null || rangeHeader.indexOf('-') == -1) { On 2011/12/05 22:00:09, yanivi wrote: > we already computed the index of '-', so you should be able to write something > as simple as: > int dash = rangeHeader.indexOf('-'); > long firstByteIndex = Long.parseLong(rangeHeader.substring(0, dash)); > long lastByteIndex = Long.parseLong(rangeHeader.substring(dash + 1)); > Implemented your comment below. http://codereview.appspot.com/5450097/diff/1/google-api-client/src/main/java/... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:260: if (rangeHeader == null || rangeHeader.indexOf('-') == -1) { On 2011/12/05 22:12:57, yanivi wrote: > On 2011/12/05 22:00:09, yanivi wrote: > > we already computed the index of '-', so you should be able to write something > > as simple as: > > int dash = rangeHeader.indexOf('-'); > > long firstByteIndex = Long.parseLong(rangeHeader.substring(0, dash)); > > long lastByteIndex = Long.parseLong(rangeHeader.substring(dash + 1)); > > > > doesn't this whole thing just reduce to something like: > > > return Long.parseLong(rangeHeader.substring(rangeHeader.indexOf('-') + 1)); Done. http://codereview.appspot.com/5450097/diff/1/google-api-client/src/main/java/... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:261: // No valid range header, start from the beginning of the file. On 2011/12/05 22:00:09, yanivi wrote: > are we really going to encounter an invalid range header? seems unnecessary to > me to be so paranoid that the Google server doesn't behave as specified. This is more to know when to stop the upload process than to check for the google servers misbehaving. http://codereview.appspot.com/5450097/diff/1/google-api-client/src/main/java/... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:378: public double getProgress() { On 2011/12/05 22:00:09, yanivi wrote: > [optional] do we really need getProgress()? isn't it sufficient to provide > getNumBytesUploaded() and getMediaContentLength()? I like this as a convenience method, the sample I write will demonstrate how to use this method. http://codereview.appspot.com/5450097/diff/1/google-api-client/src/main/java/... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:381: return uploadState.equals(UploadState.COMPLETE) ? 1 : 0; On 2011/12/05 22:00:09, yanivi wrote: > It seems really odd that getNumBytesUploaded() when UploadState.COMPLETE. > Should it return mediaContent.getLength() at that point? Can you please elaborate on this comment, I do not understand it. getProgress will return a value between 0-1, if the state is COMPLETE then 1 will be returned. http://codereview.appspot.com/5450097/diff/1/google-api-client/src/main/java/... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:383: return (double) bytesUploaded / mediaContent.getLength(); On 2011/12/05 22:00:09, yanivi wrote: > please cache mediaContent.getLength() just in case it is expensive to compute, > and provide a getter method for it Done. http://codereview.appspot.com/5450097/diff/1/google-api-client/src/main/java/... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:389: private class NotificationTask extends TimerTask { On 2011/12/05 22:00:09, yanivi wrote: > Given that there is an overhead to Java threads I'd prefer not to start threads. > Also, App Engine doesn't support threads, so we would lose progress listener > support for App Engine. Why don't we just only call listener.progressChanged > when bytesUpload changes? Done.
Sign in to reply to this message.
On 2011/12/12 15:29:38, rmistry wrote: > Supporting changes are in: > http://codereview.appspot.com/5482047/ > > http://codereview.appspot.com/5450097/diff/1/google-api-client/src/main/java/... > File > google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java > (right): > > http://codereview.appspot.com/5450097/diff/1/google-api-client/src/main/java/... > google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:117: > public MediaHttpUploader(HttpContent metadata, InputStreamContent mediaContent, > On 2011/12/05 22:12:57, yanivi wrote: > > AbstractInputStreamContent is a better choice than InputStreamContent because > > then we can also support FileContent and ByteArrayContent > > > > should we make getInputStream() public? > > Done. > > http://codereview.appspot.com/5450097/diff/1/google-api-client/src/main/java/... > google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:189: > request.setContent(ByteArrayContent.fromString(mediaContent.getType(), " ")); > On 2011/12/05 22:00:09, yanivi wrote: > > This is wrong. There should be no "Content-Type" header. Instead, the > > specification at: > > > > http://code.google.com/apis/gdata/docs/resumable_upload.html > > > > says that you should set the X-Upload-Content-Type and X-Upload-Content-Length > > on the initial POST request, regardless of whether there is metadata > > Yes I am not sure how I missed this. Maybe I thought that it was GData specific > but it is not, it is the content type and content length of the media to be > uploaded. > > http://codereview.appspot.com/5450097/diff/1/google-api-client/src/main/java/... > google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:230: > request.setContent(ByteArrayContent.fromString(mediaContent.getType(), " ")); > On 2011/12/05 22:00:09, yanivi wrote: > > this line should not be needed because there should be no Content-Type header > > according to the specification at: > > > > http://code.google.com/apis/gdata/docs/resumable_upload.html > > > > and also because HttpRequest already sets the content to " " if there is no > > content. > > Done. > > http://codereview.appspot.com/5450097/diff/1/google-api-client/src/main/java/... > google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:244: > contentInputStream.skip(bytesWritten); > On 2011/12/05 22:00:09, yanivi wrote: > > Interesting bug report from running 'mvn findbugs:gui': > > > > " > > Method ignores results of InputStream.skip() > > > > This method ignores the return value of java.io.InputStream.skip() which can > > skip multiple bytes. If the return value is not checked, the caller will > not be > > able to correctly handle the case where fewer bytes were skipped than the > caller > > requested. This is a particularly insidious kind of bug, because in many > > programs, skips from input streams usually do skip the full amount of data > > requested, causing the program to fail only sporadically. With Buffered > streams, > > however, skip() will only skip data in the buffer, and will routinely fail to > > skip the requested number of bytes. > > " > > It is strange that I do not see this msg when I run mvn findbugs:gui. Also not > sure how to handle the bug you see, I will be happy to implement any > recommendations you have. > > http://codereview.appspot.com/5450097/diff/1/google-api-client/src/main/java/... > google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:260: > if (rangeHeader == null || rangeHeader.indexOf('-') == -1) { > On 2011/12/05 22:00:09, yanivi wrote: > > we already computed the index of '-', so you should be able to write something > > as simple as: > > int dash = rangeHeader.indexOf('-'); > > long firstByteIndex = Long.parseLong(rangeHeader.substring(0, dash)); > > long lastByteIndex = Long.parseLong(rangeHeader.substring(dash + 1)); > > > > Implemented your comment below. > > http://codereview.appspot.com/5450097/diff/1/google-api-client/src/main/java/... > google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:260: > if (rangeHeader == null || rangeHeader.indexOf('-') == -1) { > On 2011/12/05 22:12:57, yanivi wrote: > > On 2011/12/05 22:00:09, yanivi wrote: > > > we already computed the index of '-', so you should be able to write > something > > > as simple as: > > > int dash = rangeHeader.indexOf('-'); > > > long firstByteIndex = Long.parseLong(rangeHeader.substring(0, dash)); > > > long lastByteIndex = Long.parseLong(rangeHeader.substring(dash + 1)); > > > > > > > doesn't this whole thing just reduce to something like: > > > > > > return Long.parseLong(rangeHeader.substring(rangeHeader.indexOf('-') + > 1)); > > Done. > > http://codereview.appspot.com/5450097/diff/1/google-api-client/src/main/java/... > google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:261: > // No valid range header, start from the beginning of the file. > On 2011/12/05 22:00:09, yanivi wrote: > > are we really going to encounter an invalid range header? seems unnecessary > to > > me to be so paranoid that the Google server doesn't behave as specified. > > This is more to know when to stop the upload process than to check for the > google servers misbehaving. > > http://codereview.appspot.com/5450097/diff/1/google-api-client/src/main/java/... > google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:378: > public double getProgress() { > On 2011/12/05 22:00:09, yanivi wrote: > > [optional] do we really need getProgress()? isn't it sufficient to provide > > getNumBytesUploaded() and getMediaContentLength()? > > I like this as a convenience method, the sample I write will demonstrate how to > use this method. > > http://codereview.appspot.com/5450097/diff/1/google-api-client/src/main/java/... > google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:381: > return uploadState.equals(UploadState.COMPLETE) ? 1 : 0; > On 2011/12/05 22:00:09, yanivi wrote: > > It seems really odd that getNumBytesUploaded() when UploadState.COMPLETE. > > Should it return mediaContent.getLength() at that point? > > Can you please elaborate on this comment, I do not understand it. getProgress > will return a value between 0-1, if the state is COMPLETE then 1 will be > returned. > > http://codereview.appspot.com/5450097/diff/1/google-api-client/src/main/java/... > google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:383: > return (double) bytesUploaded / mediaContent.getLength(); > On 2011/12/05 22:00:09, yanivi wrote: > > please cache mediaContent.getLength() just in case it is expensive to compute, > > and provide a getter method for it > > Done. > > http://codereview.appspot.com/5450097/diff/1/google-api-client/src/main/java/... > google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:389: > private class NotificationTask extends TimerTask { > On 2011/12/05 22:00:09, yanivi wrote: > > Given that there is an overhead to Java threads I'd prefer not to start > threads. > > Also, App Engine doesn't support threads, so we would lose progress listener > > support for App Engine. Why don't we just only call listener.progressChanged > > when bytesUpload changes? > > Done. Ping.
Sign in to reply to this message.
Sending the comments I've collected before going off to lunch... http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... File google-api-client/src/main/java/com/google/api/client/googleapis/MediaExponentialBackOffPolicy.java (right): http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaExponentialBackOffPolicy.java:51: public long getNextBackOffMillis() { [aside] I wonder if instead we should develop a progress listener for HttpRequest? After all, we are not actually changing the behavior ExponentialBackoffPolicy, but rather just using it to listen to error events. Anyway, this is good enough for now, but in the future it is something we can consider. http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaExponentialBackOffPolicy.java:88: public static class Builder { needs to properly extend ExponentialBackOffPolicy.Builder http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... File google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java (right): http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:29: import java.util.Timer; Description Resource Path Location Type The import java.util.Timer is never used MediaHttpUploader.java /google-api-client/src/main/java/com/google/api/client/googleapis line 29 Java Problem The import java.util.TimerTask is never used MediaHttpUploader.java /google-api-client/src/main/java/com/google/api/client/googleapis line 30 Java Problem http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:45: // TODO(rmistry): Add support for resumable media update. [optional] Consider opening feature requests on the issue tracker since they are high-level features. http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:47: public final class MediaHttpUploader { this could really benefit from some unit tests, including progress listener notifications http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:82: /** The HTTP request object that is currently used to send upload requests. */ or @{code null} before {@link #upload} http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:85: /** An Input stream of the HTTP media content. */ or @{code null} before {@link #upload} http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:89: * The back off policy to use when executing HTTP requests. {@link MediaExponentialBackOffPolicy} or {@code null} for none. If we have exponential backoff disabled by default for HttpRequest, should we disable it by default here too? http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:115: public MediaHttpUploader(HttpContent metadata, AbstractInputStreamContent mediaContent, I am unsure about AbstractInputStreamContent since we are not actually using it as intended to write the whole content. Should we instead just pass InputStream and String content type? http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:115: public MediaHttpUploader(HttpContent metadata, AbstractInputStreamContent mediaContent, At what point do we close the input stream? Should we even be responsible for closing the input stream? Either way, it is very important for the JavaDoc to be very clear about this. http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:116: HttpRequestFactory requestFactory) { The general pattern we've followed is to have HttpTransport and HttpRequestInitializer as the inputs, e.g. GoogleClient. I'm okay with either style, but I'd prefer that we pick one style and stick to it. http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:120: this.mediaContentLength = mediaContent.getLength(); instead please use lazy initialization to compute this, and throw the IOException as needed http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:179: setUploadState(UploadState.COMPLETE); we should send progressChanged notification after it completes http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:382: public double getProgress() throws IOException { Description Resource Path Location Type The declared exception IOException is not actually thrown by the method getProgress() from type MediaHttpUploader MediaHttpUploader.java /google-api-client/src/main/java/com/google/api/client/googleapis line 382 Java Problem http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... File google-api-client/src/main/java/com/google/api/client/googleapis/ProgressListener.java (right): http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/ProgressListener.java:17: import com.google.common.base.Preconditions; Description Resource Path Location Type The import com.google.common.base.Preconditions is never used ProgressListener.java /google-api-client/src/main/java/com/google/api/client/googleapis line 17 Java Problem http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/ProgressListener.java:22: * An abstract class for receiving progress notifications for uploads. Interface, not class http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/ProgressListener.java:55: * upload completes. Progress notifications are not made while an upload has not been started or "while an upload has not been started" -> "before an upload starts" http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/ProgressListener.java:56: * after it completes. Instead, I'd expect to receive one notification before it starts, one notification after it completes, and multiple notifications while it is in progress.
Sign in to reply to this message.
Review completed! This should keep you busy ;) http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... File google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java (right): http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:45: // TODO(rmistry): Add support for resumable media update. Actually this is easy to implement: just provide an option to specify if this is a POST or a PUT (perhaps an HttpMethod parameter). Then just use that HttpMethod in building the initial upload request. Otherwise, as far as I can tell they are an identical protocol. http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:46: // TODO(rmistry): Add ability to customize the HttpHeader on the initiation request. This is also easy to implement: just add a "GoogleHeaders headers" field and a "GoogleHeaders getHeaders()" method, and then call "request.setHeaders(headers)" in executeUploadInitiation. Done. http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:53: NOT_STARTED, IN_PROGRESS, COMPLETE would help to add a status for metadata, so that IN_PROGRESS would refer to media only http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:65: public static final long DEFAULT_MAX_CHUNK_SIZE = 10 * MB; I recommend renaming it to DEFAULT_CHUNK_SIZE http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:68: private final MethodOverride methodOverride = new MethodOverride(); Why bother storing the methodOverride instance? maybe cleaner to construct it every time? In general I'd prefer minimizing the number of fields in a class. http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:83: private HttpRequest currentRequest; Somehow doesn't feel right to me to store the current request. I'd rather build the HTTP request object every time based on stored information. http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:115: public MediaHttpUploader(HttpContent metadata, AbstractInputStreamContent mediaContent, Should we instead use a setter for metadata and not include it as a parameter? Ideally only required parameters should be in the constructor. Similarly for HttpRequestInitializer. http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:136: public HttpResponse upload(GenericUrl initiationRequestUrl) throws IOException { what happens if someone calls upload twice? we should either throw an exception (probably preferred), or fully support it. http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:150: // Upload the media content in chunks. When the bytesWritten is 0 it means that the upload has remove the second sentence http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:160: response = currentRequest.execute(); if we get a success response, we should just immediately do: bytesUploaded = mediaContentLength; setUploadState(UploadState.COMPLETE); if (progressListener != null) { progressListener.progressChanged(this); } return response; then you can just while(true) above http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:163: if (response.getStatusCode() != 308) { can you put a TODO to use HttpRequest.setSuppressExceptionOnError(true) when it is ready? See: http://codereview.appspot.com/5447058/diff/17001/google-http-client/src/main/... similarly below http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:169: String updatedUploadUrl = response.getHeaders().getLocation(); I'd feel better about storing bytesWritten and uploadUrl as fields http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:189: HttpRequest request = requestFactory.buildPostRequest(initiationRequestUrl, null); second parameter should be metadata, then remove the next 3 lines http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:193: request.getHeaders().setUploadContentType(mediaContent.getType()); As noted in the other changeset, lets move setUploadContentType and setUploadContentLength to GoogleHeaders instead. Note that you can do request.setHeaders(...). alternative, just do request.getHeaders().put("X-Upload-Content-Type", ...) directly http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:194: request.getHeaders().setUploadContentLength(String.valueOf(mediaContentLength)); should we perhaps make the parameter type in GoogleHeaders a long instead of a String http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:206: int blockSize = (int) Math.min(getChunkSize(), mediaContentLength - bytesWritten); in general i prefer a consistent style that we don't call getters from internal code and instead access the fields directly http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:207: byte[] block = new byte[blockSize]; In theory this use a lot of memory if chunkSize is large. Ideally we would want to write directly from input stream to output. Essentially what we ideally want is something like what's implemented in AbstractInputStreamContent.write(OutputStream), except with the ability to only read a range of bytes from input stream. We could extract that capability from AbstractInputstreamContent into a utility method and call it from here. http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:208: // Mark the current positiion in case we need to retry the request. positiion->position http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:229: public void serverErrorCallback() throws IOException { Suggestion: if this is too complicated, I suggest just implementing this without the error handling or exponential backoff for your first implementation. http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:229: public void serverErrorCallback() throws IOException { Might be useful to add a TODO to handle timeouts similarly to the way we handle 50x errors. The resumable upload spec mentions this. http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:236: request.setAllowEmptyContent(true); Really??? Does it have to be 0? Have you tried it? My understanding is that Google's front end will give a 411 error under a certain JDK for a PUT with no content. http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:237: request.getHeaders().setContentLength("0"); I prefer that we use ByteArrayContent(null, new byte[0]) http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:257: // The current position of the input stream is likely incorrect because the upload was I'm lost. I don't understand what you are doing here. reset() will set it to the position last read. All you need to do is substract previous bytesWritten from new bytesWritten and send that to skip method. Anything else will put you in the incorrect spot. You might want to test this out to make sure it is doing the right thing. http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:282: if (rangeHeader == null || rangeHeader.indexOf('-') == -1) { Remove the (rangeHeader.indexOf('-') == -1) check, since 1. we can be safe the Google APIs follow the specification, and 2. it will work without it anyway http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:299: public HttpRequestFactory getRequestFactory() { if we change the inputs to HttpTransport+HttpRequestInitializer, we may want to similarly remove getRequestFactory() and add getHttpTransport() and getHttpRequestInitializer() http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:307: public void setMediaBackOffPolicy(MediaExponentialBackOffPolicy mediaBackOffPolicy) { general style we've followed is that setters return this http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:337: public void setChunkSize(long chunkSize) { Should chunk size be an int instead? I wonder if perhaps we should not support chunks above 2GB http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:338: this.chunkSize = chunkSize; lets enforce chunk size >= 1 http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:363: void setUploadState(UploadState uploadState) { seems sort of silly to write a package-private setter method http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... File google-api-client/src/main/java/com/google/api/client/googleapis/services/GoogleClient.java (left): http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/services/GoogleClient.java:97: protected HttpRequest buildHttpRequest( deprecate, but don't remove this method
Sign in to reply to this message.
http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... File google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java (right): http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:266: contentInputStream.skip(skipValue); check that the return value == skipValue. otherwise, throw an exception?
Sign in to reply to this message.
http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... File google-api-client/src/main/java/com/google/api/client/googleapis/MediaExponentialBackOffPolicy.java (right): http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaExponentialBackOffPolicy.java:51: public long getNextBackOffMillis() { On 2012/01/13 17:35:07, yanivi wrote: > [aside] I wonder if instead we should develop a progress listener for > HttpRequest? After all, we are not actually changing the behavior > ExponentialBackoffPolicy, but rather just using it to listen to error events. > Anyway, this is good enough for now, but in the future it is something we can > consider. ok http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaExponentialBackOffPolicy.java:88: public static class Builder { On 2012/01/13 17:35:07, yanivi wrote: > needs to properly extend ExponentialBackOffPolicy.Builder Done. http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... File google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java (right): http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:29: import java.util.Timer; On 2012/01/13 17:35:07, yanivi wrote: > Description Resource Path Location Type > The import java.util.Timer is never > used MediaHttpUploader.java /google-api-client/src/main/java/com/google/api/client/googleapis line > 29 Java Problem > The import java.util.TimerTask is never > used MediaHttpUploader.java /google-api-client/src/main/java/com/google/api/client/googleapis line > 30 Java Problem Done. http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:45: // TODO(rmistry): Add support for resumable media update. On 2012/01/13 20:45:30, yanivi wrote: > Actually this is easy to implement: just provide an option to specify if this is > a POST or a PUT (perhaps an HttpMethod parameter). Then just use that > HttpMethod in building the initial upload request. Otherwise, as far as I can > tell they are an identical protocol. Done. http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:45: // TODO(rmistry): Add support for resumable media update. On 2012/01/13 17:35:07, yanivi wrote: > [optional] Consider opening feature requests on the issue tracker since they are > high-level features. Filed: http://code.google.com/p/google-api-java-client/issues/detail?id=391 http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:46: // TODO(rmistry): Add ability to customize the HttpHeader on the initiation request. On 2012/01/13 20:45:30, yanivi wrote: > This is also easy to implement: just add a "GoogleHeaders headers" field and a > "GoogleHeaders getHeaders()" method, and then call "request.setHeaders(headers)" > in executeUploadInitiation. Done. Done. http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:47: public final class MediaHttpUploader { On 2012/01/13 17:35:07, yanivi wrote: > this could really benefit from some unit tests, including progress listener > notifications Done. http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:53: NOT_STARTED, IN_PROGRESS, COMPLETE On 2012/01/13 20:45:30, yanivi wrote: > would help to add a status for metadata, so that IN_PROGRESS would refer to > media only Instead of one for metadata (which is optional) I added one for INITIATION_STARTED and INITIATION_COMPLETE this can apply to with or without metadata. http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:65: public static final long DEFAULT_MAX_CHUNK_SIZE = 10 * MB; On 2012/01/13 20:45:30, yanivi wrote: > I recommend renaming it to DEFAULT_CHUNK_SIZE Done. http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:68: private final MethodOverride methodOverride = new MethodOverride(); On 2012/01/13 20:45:30, yanivi wrote: > Why bother storing the methodOverride instance? maybe cleaner to construct it > every time? In general I'd prefer minimizing the number of fields in a class. Done. http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:82: /** The HTTP request object that is currently used to send upload requests. */ On 2012/01/13 17:35:07, yanivi wrote: > or @{code null} before {@link #upload} Done. http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:83: private HttpRequest currentRequest; On 2012/01/13 20:45:30, yanivi wrote: > Somehow doesn't feel right to me to store the current request. I'd rather build > the HTTP request object every time based on stored information. I understand what you mean, but in this case we have to store the current request because if there is a server error in HttpRequest.execute #serverErrorCallback needs to be able to modify the current request and fix it before it is retried, thus it needs to be able to get a handle on the current request. http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:85: /** An Input stream of the HTTP media content. */ On 2012/01/13 17:35:07, yanivi wrote: > or @{code null} before {@link #upload} Done. http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:89: * The back off policy to use when executing HTTP requests. {@link MediaExponentialBackOffPolicy} On 2012/01/13 17:35:07, yanivi wrote: > or {@code null} for none. If we have exponential backoff disabled by default > for HttpRequest, should we disable it by default here too? The way this is designed we require exponential backoff to be enabled to be able to recover from server errors. I think to conform to the resumable upload protocol we should enable it by default. Alternatively, I can make the sleep values in MediaExponentialBackOffPolicy very small. [aside] I plan to document every new major feature (backoff, resumable upload, batch etc) in the project wiki with snippets and class diagrams (similar to what I did in the presentations). The documentation should make it clear what the behavior of MediaHttpUploader is and how to disable MediaExponentialBackOffPolicy if it is not required. http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:115: public MediaHttpUploader(HttpContent metadata, AbstractInputStreamContent mediaContent, On 2012/01/13 20:45:30, yanivi wrote: > Should we instead use a setter for metadata and not include it as a parameter? > Ideally only required parameters should be in the constructor. Similarly for > HttpRequestInitializer. Done. http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:115: public MediaHttpUploader(HttpContent metadata, AbstractInputStreamContent mediaContent, On 2012/01/13 17:35:07, yanivi wrote: > I am unsure about AbstractInputStreamContent since we are not actually using it > as intended to write the whole content. Should we instead just pass InputStream > and String content type? The other advantage of AbstractInputStreamContent is it also contains the length of the media content. If we decide to remove AbstractInputStreamContent then we will have to pass in InputStream, String content type, long length. I think AbstractInputStreamContent is a handy container for all that information. http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:115: public MediaHttpUploader(HttpContent metadata, AbstractInputStreamContent mediaContent, On 2012/01/13 17:35:07, yanivi wrote: > At what point do we close the input stream? Should we even be responsible for > closing the input stream? Either way, it is very important for the JavaDoc to > be very clear about this. I am now closing the contentInputStream if the response is successful and I added some javadoc in the constructor. http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:116: HttpRequestFactory requestFactory) { On 2012/01/13 17:35:07, yanivi wrote: > The general pattern we've followed is to have HttpTransport and > HttpRequestInitializer as the inputs, e.g. GoogleClient. I'm okay with either > style, but I'd prefer that we pick one style and stick to it. This class will be called from the generated libraries which extend GoogleClient (which extends JsonHttpClient). JsonHttpClient does not expose HttpTransport and HttpRequestInitializer but it does expose HttpRequestFactory which is why it is simpler to just pass in HttpRequestFactory into this constructor. http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:120: this.mediaContentLength = mediaContent.getLength(); On 2012/01/13 17:35:07, yanivi wrote: > instead please use lazy initialization to compute this, and throw the > IOException as needed Done. http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:136: public HttpResponse upload(GenericUrl initiationRequestUrl) throws IOException { On 2012/01/13 20:45:30, yanivi wrote: > what happens if someone calls upload twice? we should either throw an exception > (probably preferred), or fully support it. Done. http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:150: // Upload the media content in chunks. When the bytesWritten is 0 it means that the upload has On 2012/01/13 20:45:30, yanivi wrote: > remove the second sentence Done. http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:160: response = currentRequest.execute(); On 2012/01/13 20:45:30, yanivi wrote: > if we get a success response, we should just immediately do: > > bytesUploaded = mediaContentLength; > setUploadState(UploadState.COMPLETE); > if (progressListener != null) { > progressListener.progressChanged(this); > } > return response; > > then you can just while(true) above Done. http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:163: if (response.getStatusCode() != 308) { On 2012/01/13 20:45:30, yanivi wrote: > can you put a TODO to use HttpRequest.setSuppressExceptionOnError(true) when it > is ready? See: > > http://codereview.appspot.com/5447058/diff/17001/google-http-client/src/main/... > > similarly below Done. http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:169: String updatedUploadUrl = response.getHeaders().getLocation(); On 2012/01/13 20:45:30, yanivi wrote: > I'd feel better about storing bytesWritten and uploadUrl as fields Do you mean as opposed to storing the current request? Please see my note above for why we need the current request. http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:179: setUploadState(UploadState.COMPLETE); On 2012/01/13 17:35:07, yanivi wrote: > we should send progressChanged notification after it completes Done. http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:189: HttpRequest request = requestFactory.buildPostRequest(initiationRequestUrl, null); On 2012/01/13 20:45:30, yanivi wrote: > second parameter should be metadata, then remove the next 3 lines Done. http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:193: request.getHeaders().setUploadContentType(mediaContent.getType()); On 2012/01/13 20:45:30, yanivi wrote: > As noted in the other changeset, lets move setUploadContentType and > setUploadContentLength to GoogleHeaders instead. Note that you can do > request.setHeaders(...). > > alternative, just do request.getHeaders().put("X-Upload-Content-Type", ...) > directly Moved to GoogleHeaders. Done. http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:194: request.getHeaders().setUploadContentLength(String.valueOf(mediaContentLength)); On 2012/01/13 20:45:30, yanivi wrote: > should we perhaps make the parameter type in GoogleHeaders a long instead of a > String Done. http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:206: int blockSize = (int) Math.min(getChunkSize(), mediaContentLength - bytesWritten); On 2012/01/13 20:45:30, yanivi wrote: > in general i prefer a consistent style that we don't call getters from internal > code and instead access the fields directly Done. http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:207: byte[] block = new byte[blockSize]; On 2012/01/13 20:45:30, yanivi wrote: > In theory this use a lot of memory if chunkSize is large. Ideally we would want > to write directly from input stream to output. Essentially what we ideally want > is something like what's implemented in > AbstractInputStreamContent.write(OutputStream), except with the ability to only > read a range of bytes from input stream. We could extract that capability from > AbstractInputstreamContent into a utility method and call it from here. chunk size has been changed to an int, is this still a concern with maximum 2gb? Alternatively, we could enforce a non modifiable chunk size of 10mb. If callers are not happy with the chunk size and would like to do a direct upload they could call the code that will come in: http://code.google.com/p/google-api-java-client/issues/detail?id=391 http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:208: // Mark the current positiion in case we need to retry the request. On 2012/01/13 20:45:30, yanivi wrote: > positiion->position Done. http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:229: public void serverErrorCallback() throws IOException { On 2012/01/13 20:45:30, yanivi wrote: > Might be useful to add a TODO to handle timeouts similarly to the way we handle > 50x errors. The resumable upload spec mentions this. Done. http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:229: public void serverErrorCallback() throws IOException { On 2012/01/13 20:45:30, yanivi wrote: > Suggestion: if this is too complicated, I suggest just implementing this without > the error handling or exponential backoff for your first implementation. This implementation is working, I have tested it by sending a request with an incorrect range header and have seen the serverErrorCallback method correct the range in the subsequent requests. http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:236: request.setAllowEmptyContent(true); On 2012/01/13 20:45:30, yanivi wrote: > Really??? Does it have to be 0? Have you tried it? My understanding is that > Google's front end will give a 411 error under a certain JDK for a PUT with no > content. This is what I get when it is not 0: CONFIG: -------------- REQUEST -------------- PUT xyz Accept-Encoding: gzip Authorization: <Not Logged> Content-Range: bytes */62763 User-Agent: Google-HTTP-Java-Client/1.7.0-beta-SNAPSHOT (gzip) Content-Length: 1 Jan 18, 2012 10:03:13 AM com.google.api.client.http.HttpResponse <init> CONFIG: -------------- RESPONSE -------------- HTTP/1.0 400 Bad Request Date: Wed, 18 Jan 2012 18:03:13 GMT Content-Length: 925 Content-Type: text/html; charset=UTF-8 Server: GFE/2.0 http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:237: request.getHeaders().setContentLength("0"); On 2012/01/13 20:45:30, yanivi wrote: > I prefer that we use ByteArrayContent(null, new byte[0]) Done. http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:257: // The current position of the input stream is likely incorrect because the upload was On 2012/01/13 20:45:30, yanivi wrote: > I'm lost. I don't understand what you are doing here. reset() will set it to > the position last read. All you need to do is substract previous bytesWritten > from new bytesWritten and send that to skip method. Anything else will put you > in the incorrect spot. You might want to test this out to make sure it is doing > the right thing. Done. I did test out what I previously had but I think my test case was faulty. I added a unit test for this and improved the way I simulate server errors by hand. http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:266: contentInputStream.skip(skipValue); On 2012/01/13 20:57:06, yanivi wrote: > check that the return value == skipValue. otherwise, throw an exception? Done. http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:282: if (rangeHeader == null || rangeHeader.indexOf('-') == -1) { On 2012/01/13 20:45:30, yanivi wrote: > Remove the (rangeHeader.indexOf('-') == -1) check, since 1. we can be safe the > Google APIs follow the specification, and 2. it will work without it anyway Done. http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:299: public HttpRequestFactory getRequestFactory() { On 2012/01/13 20:45:30, yanivi wrote: > if we change the inputs to HttpTransport+HttpRequestInitializer, we may want to > similarly remove getRequestFactory() and add getHttpTransport() and > getHttpRequestInitializer() Please see note above for why I kept request factory. http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:307: public void setMediaBackOffPolicy(MediaExponentialBackOffPolicy mediaBackOffPolicy) { On 2012/01/13 20:45:30, yanivi wrote: > general style we've followed is that setters return this Done. http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:337: public void setChunkSize(long chunkSize) { On 2012/01/13 20:45:30, yanivi wrote: > Should chunk size be an int instead? I wonder if perhaps we should not support > chunks above 2GB I think that makes sense. Done. http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:338: this.chunkSize = chunkSize; On 2012/01/13 20:45:30, yanivi wrote: > lets enforce chunk size >= 1 Done. http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:363: void setUploadState(UploadState uploadState) { On 2012/01/13 20:45:30, yanivi wrote: > seems sort of silly to write a package-private setter method Changed this to: private void updateStateAndNotifyListener(UploadState uploadState) http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:382: public double getProgress() throws IOException { On 2012/01/13 17:35:07, yanivi wrote: > Description Resource Path Location Type > The declared exception IOException is not actually thrown by the method > getProgress() from type > MediaHttpUploader MediaHttpUploader.java /google-api-client/src/main/java/com/google/api/client/googleapis line > 382 Java Problem Done. http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... File google-api-client/src/main/java/com/google/api/client/googleapis/ProgressListener.java (right): http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/ProgressListener.java:17: import com.google.common.base.Preconditions; On 2012/01/13 17:35:07, yanivi wrote: > Description Resource Path Location Type > The import com.google.common.base.Preconditions is never > used ProgressListener.java /google-api-client/src/main/java/com/google/api/client/googleapis line > 17 Java Problem Done. http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/ProgressListener.java:22: * An abstract class for receiving progress notifications for uploads. On 2012/01/13 17:35:07, yanivi wrote: > Interface, not class Done. http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/ProgressListener.java:55: * upload completes. Progress notifications are not made while an upload has not been started or On 2012/01/13 17:35:07, yanivi wrote: > "while an upload has not been started" -> "before an upload starts" Done. http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/ProgressListener.java:56: * after it completes. On 2012/01/13 17:35:07, yanivi wrote: > Instead, I'd expect to receive one notification before it starts, one > notification after it completes, and multiple notifications while it is in > progress. Done. http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... File google-api-client/src/main/java/com/google/api/client/googleapis/services/GoogleClient.java (left): http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/services/GoogleClient.java:97: protected HttpRequest buildHttpRequest( On 2012/01/13 20:45:30, yanivi wrote: > deprecate, but don't remove this method Done.
Sign in to reply to this message.
Not done reviewing, but I want to send what I've collected so far before going home... http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... File google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java (right): http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:116: HttpRequestFactory requestFactory) { On 2012/01/19 16:20:18, rmistry wrote: > On 2012/01/13 17:35:07, yanivi wrote: > > The general pattern we've followed is to have HttpTransport and > > HttpRequestInitializer as the inputs, e.g. GoogleClient. I'm okay with either > > style, but I'd prefer that we pick one style and stick to it. > > This class will be called from the generated libraries which extend GoogleClient > (which extends JsonHttpClient). JsonHttpClient does not expose HttpTransport and > HttpRequestInitializer but it does expose HttpRequestFactory which is why it is > simpler to just pass in HttpRequestFactory into this constructor. That's not a good argument for it because HttpRequestFactory has getInitializer() and getTransport(). Also, keep in mind that I have an explicit goal to make MediaHttpUploader to be usable without the generated layer, especially to access GData APIs. So I do think we need to think of MediaHttpUploader in the more general context. http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:124: Preconditions.checkArgument(this.mediaContentLength != -1); lets move this precondition into getMediaContentLength() http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:207: byte[] block = new byte[blockSize]; On 2012/01/19 16:20:18, rmistry wrote: > On 2012/01/13 20:45:30, yanivi wrote: > > In theory this use a lot of memory if chunkSize is large. Ideally we would > want > > to write directly from input stream to output. Essentially what we ideally > want > > is something like what's implemented in > > AbstractInputStreamContent.write(OutputStream), except with the ability to > only > > read a range of bytes from input stream. We could extract that capability > from > > AbstractInputstreamContent into a utility method and call it from here. > > chunk size has been changed to an int, is this still a concern with maximum 2gb? > Alternatively, we could enforce a non modifiable chunk size of 10mb. If callers > are not happy with the chunk size and would like to do a direct upload they > could call the code that will come in: > http://code.google.com/p/google-api-java-client/issues/detail?id=391 Yes, I still think it is a potentially serious problem. Android developers have complained about far smaller in-memory chunk sizes. http://codereview.appspot.com/5450097/diff/19003/google-api-client/src/main/j... File google-api-client/src/main/java/com/google/api/client/googleapis/GoogleHeaders.java (right): http://codereview.appspot.com/5450097/diff/19003/google-api-client/src/main/j... google-api-client/src/main/java/com/google/api/client/googleapis/GoogleHeaders.java:3: * [optional] please try to avoid whitespace changes if possible http://codereview.appspot.com/5450097/diff/19003/google-api-client/src/main/j... google-api-client/src/main/java/com/google/api/client/googleapis/GoogleHeaders.java:118: public long uploadContentLength; private also use getters and setters basically, your code from HttpHeaders should be copied as is. [optional] If you are feeling motivated, please also update the other headers to follow our new code style convention. and if you're really feeling adventurous, please also return this from setter methods. http://codereview.appspot.com/5450097/diff/19003/google-api-client/src/main/j... google-api-client/src/main/java/com/google/api/client/googleapis/GoogleHeaders.java:153: * Sets the {@code "Content-Length"} header or {@code null} for none. X-Upload-Content-Length http://codereview.appspot.com/5450097/diff/19003/google-api-client/src/main/j... File google-api-client/src/main/java/com/google/api/client/googleapis/MediaExponentialBackOffPolicy.java (right): http://codereview.appspot.com/5450097/diff/19003/google-api-client/src/main/j... google-api-client/src/main/java/com/google/api/client/googleapis/MediaExponentialBackOffPolicy.java:104: public MediaExponentialBackOffPolicy build() { Your implementation is completely wrong, because you are missing the ability to use all of the properties inherited from ExponentialBackOffPolicy.Builder to initialize the MediaExponentialBackOffPolicy. You will probably need a constructor to match. [aside] I wish we didn't need to extend ExponentialBackOffPolicy (see my aside from earlier) You know another option is to just not make MediaExponentialBackOffPolicy public at all. That way you wouldn't need to be so user friendly. If so, you might just provide a way to enable/disable exponential backoff with a boolean. http://codereview.appspot.com/5450097/diff/19003/google-api-client/src/main/j... File google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java (right): http://codereview.appspot.com/5450097/diff/19003/google-api-client/src/main/j... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:49: NOT_STARTED, INITIATION_STARTED, INITIATION_COMPLETE, MEDIA_IN_PROGRESS, MEDIA_COMPLETE Missing JavaDoc on the enum values. It's just really non-obvious to me what each one means. http://codereview.appspot.com/5450097/diff/19003/google-api-client/src/main/j... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:72: /** The length of the HTTP media content. */ specify that this is 0 before lazily initialized in {@link #getMediaContentLength()} http://codereview.appspot.com/5450097/diff/19003/google-api-client/src/main/j... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:173: // TODO(rmistry): Use HttpRequest.setSuppressExceptionOnError(true) when it is submitted. it is now called setThrowExceptionOnExecuteError http://codereview.appspot.com/5450097/diff/19003/google-api-client/src/main/j... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:206: initiationHeaders.setUploadContentType(mediaContent.getType()); need MethodOverride in case initiationMethod is PUT http://codereview.appspot.com/5450097/diff/19003/google-api-client/src/main/j... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:395: public GoogleHeaders getInitiationHeaders() { please also provide setInitiationHeaders() http://codereview.appspot.com/5450097/diff/19003/google-api-client/src/test/j... File google-api-client/src/test/java/com/google/api/client/googleapis/MediaHttpUploaderTest.java (right): http://codereview.appspot.com/5450097/diff/19003/google-api-client/src/test/j... google-api-client/src/test/java/com/google/api/client/googleapis/MediaHttpUploaderTest.java:135: @Override remove @Override
Sign in to reply to this message.
http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... File google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java (right): http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:116: HttpRequestFactory requestFactory) { On 2012/01/20 21:46:21, yanivi wrote: > On 2012/01/19 16:20:18, rmistry wrote: > > On 2012/01/13 17:35:07, yanivi wrote: > > > The general pattern we've followed is to have HttpTransport and > > > HttpRequestInitializer as the inputs, e.g. GoogleClient. I'm okay with > either > > > style, but I'd prefer that we pick one style and stick to it. > > > > This class will be called from the generated libraries which extend > GoogleClient > > (which extends JsonHttpClient). JsonHttpClient does not expose HttpTransport > and > > HttpRequestInitializer but it does expose HttpRequestFactory which is why it > is > > simpler to just pass in HttpRequestFactory into this constructor. > > That's not a good argument for it because HttpRequestFactory has > getInitializer() and getTransport(). > > Also, keep in mind that I have an explicit goal to make MediaHttpUploader to be > usable without the generated layer, especially to access GData APIs. So I do > think we need to think of MediaHttpUploader in the more general context. Yes you are right I forgot about getInitializer() and getTransport(). Done. http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:207: byte[] block = new byte[blockSize]; On 2012/01/20 21:46:21, yanivi wrote: > On 2012/01/19 16:20:18, rmistry wrote: > > On 2012/01/13 20:45:30, yanivi wrote: > > > In theory this use a lot of memory if chunkSize is large. Ideally we would > > want > > > to write directly from input stream to output. Essentially what we ideally > > want > > > is something like what's implemented in > > > AbstractInputStreamContent.write(OutputStream), except with the ability to > > only > > > read a range of bytes from input stream. We could extract that capability > > from > > > AbstractInputstreamContent into a utility method and call it from here. > > > > chunk size has been changed to an int, is this still a concern with maximum > 2gb? > > Alternatively, we could enforce a non modifiable chunk size of 10mb. If > callers > > are not happy with the chunk size and would like to do a direct upload they > > could call the code that will come in: > > http://code.google.com/p/google-api-java-client/issues/detail?id=391 > > Yes, I still think it is a potentially serious problem. Android developers have > complained about far smaller in-memory chunk sizes. Ok, I understand. Could you explain how the implementation would work? I understand how to write it to an OutputStream (using your example above with AbstractInputStreamContent.write) but do not see how to convert that into an HttpContent. http://codereview.appspot.com/5450097/diff/19003/google-api-client/src/main/j... File google-api-client/src/main/java/com/google/api/client/googleapis/GoogleHeaders.java (right): http://codereview.appspot.com/5450097/diff/19003/google-api-client/src/main/j... google-api-client/src/main/java/com/google/api/client/googleapis/GoogleHeaders.java:3: * On 2012/01/20 21:46:22, yanivi wrote: > [optional] please try to avoid whitespace changes if possible I apologize for them, they can get distracting. Eclipse made the changes automatically. http://codereview.appspot.com/5450097/diff/19003/google-api-client/src/main/j... google-api-client/src/main/java/com/google/api/client/googleapis/GoogleHeaders.java:118: public long uploadContentLength; On 2012/01/20 21:46:22, yanivi wrote: > private > > also use getters and setters > > basically, your code from HttpHeaders should be copied as is. Done. > > [optional] If you are feeling motivated, please also update the other headers to > follow our new code style convention. > > and if you're really feeling adventurous, please also return this from setter > methods. If you do not mind I will do this in a separate CL. Filed: http://code.google.com/p/google-api-java-client/issues/detail?id=396 http://codereview.appspot.com/5450097/diff/19003/google-api-client/src/main/j... google-api-client/src/main/java/com/google/api/client/googleapis/GoogleHeaders.java:153: * Sets the {@code "Content-Length"} header or {@code null} for none. On 2012/01/20 21:46:22, yanivi wrote: > X-Upload-Content-Length Done. http://codereview.appspot.com/5450097/diff/19003/google-api-client/src/main/j... File google-api-client/src/main/java/com/google/api/client/googleapis/MediaExponentialBackOffPolicy.java (right): http://codereview.appspot.com/5450097/diff/19003/google-api-client/src/main/j... google-api-client/src/main/java/com/google/api/client/googleapis/MediaExponentialBackOffPolicy.java:104: public MediaExponentialBackOffPolicy build() { On 2012/01/20 21:46:22, yanivi wrote: > Your implementation is completely wrong, because you are missing the ability to > use all of the properties inherited from ExponentialBackOffPolicy.Builder to > initialize the MediaExponentialBackOffPolicy. You will probably need a > constructor to match. > > [aside] I wish we didn't need to extend ExponentialBackOffPolicy (see my aside > from earlier) > > You know another option is to just not make MediaExponentialBackOffPolicy public > at all. That way you wouldn't need to be so user friendly. If so, you might > just provide a way to enable/disable exponential backoff with a boolean. I like the approach of not making MediaExponentialBackOffPolicy public. Removed the Builder since it does not need to be user friendly anymore and added backOffPolicyEnabled in MediaHttpUploader. http://codereview.appspot.com/5450097/diff/19003/google-api-client/src/main/j... File google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java (right): http://codereview.appspot.com/5450097/diff/19003/google-api-client/src/main/j... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:49: NOT_STARTED, INITIATION_STARTED, INITIATION_COMPLETE, MEDIA_IN_PROGRESS, MEDIA_COMPLETE On 2012/01/20 21:46:22, yanivi wrote: > Missing JavaDoc on the enum values. It's just really non-obvious to me what > each one means. Done. http://codereview.appspot.com/5450097/diff/19003/google-api-client/src/main/j... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:72: /** The length of the HTTP media content. */ On 2012/01/20 21:46:22, yanivi wrote: > specify that this is 0 before lazily initialized in {@link > #getMediaContentLength()} Done. http://codereview.appspot.com/5450097/diff/19003/google-api-client/src/main/j... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:173: // TODO(rmistry): Use HttpRequest.setSuppressExceptionOnError(true) when it is submitted. On 2012/01/20 21:46:22, yanivi wrote: > it is now called setThrowExceptionOnExecuteError Done. http://codereview.appspot.com/5450097/diff/19003/google-api-client/src/main/j... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:206: initiationHeaders.setUploadContentType(mediaContent.getType()); On 2012/01/20 21:46:22, yanivi wrote: > need MethodOverride in case initiationMethod is PUT Done. http://codereview.appspot.com/5450097/diff/19003/google-api-client/src/main/j... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:395: public GoogleHeaders getInitiationHeaders() { On 2012/01/20 21:46:22, yanivi wrote: > please also provide setInitiationHeaders() Done. http://codereview.appspot.com/5450097/diff/19003/google-api-client/src/test/j... File google-api-client/src/test/java/com/google/api/client/googleapis/MediaHttpUploaderTest.java (right): http://codereview.appspot.com/5450097/diff/19003/google-api-client/src/test/j... google-api-client/src/test/java/com/google/api/client/googleapis/MediaHttpUploaderTest.java:135: @Override On 2012/01/20 21:46:22, yanivi wrote: > remove @Override Done.
Sign in to reply to this message.
Ping.
Sign in to reply to this message.
Done reviewing. http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... File google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java (right): http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:124: Preconditions.checkArgument(this.mediaContentLength != -1); On 2012/01/20 21:46:21, yanivi wrote: > lets move this precondition into getMediaContentLength() Ping http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:207: byte[] block = new byte[blockSize]; On 2012/01/23 22:20:28, rmistry wrote: > On 2012/01/20 21:46:21, yanivi wrote: > > On 2012/01/19 16:20:18, rmistry wrote: > > > On 2012/01/13 20:45:30, yanivi wrote: > > > > In theory this use a lot of memory if chunkSize is large. Ideally we > would > > > want > > > > to write directly from input stream to output. Essentially what we > ideally > > > want > > > > is something like what's implemented in > > > > AbstractInputStreamContent.write(OutputStream), except with the ability to > > > only > > > > read a range of bytes from input stream. We could extract that capability > > > from > > > > AbstractInputstreamContent into a utility method and call it from here. > > > > > > chunk size has been changed to an int, is this still a concern with maximum > > 2gb? > > > Alternatively, we could enforce a non modifiable chunk size of 10mb. If > > callers > > > are not happy with the chunk size and would like to do a direct upload they > > > could call the code that will come in: > > > http://code.google.com/p/google-api-java-client/issues/detail?id=391 > > > > Yes, I still think it is a potentially serious problem. Android developers > have > > complained about far smaller in-memory chunk sizes. > > Ok, I understand. Could you explain how the implementation would work? I > understand how to write it to an OutputStream (using your example above with > AbstractInputStreamContent.write) but do not see how to convert that into an > HttpContent. I haven't looked at the details yet, but I'd think you should be able to just use new InputStreamContent(contentInputStream) with setLength(blockSize). So your implementation might actually be simpler with my proposal. The only obvious catch I see is that AbstractInputStreamContent.writeTo actually closes the input stream. We should probably add an option to AbstractInputStreamContent disable closing the input stream (though it should be enabled by default). http://codereview.appspot.com/5450097/diff/19003/google-api-client/src/main/j... File google-api-client/src/main/java/com/google/api/client/googleapis/services/GoogleClient.java (right): http://codereview.appspot.com/5450097/diff/19003/google-api-client/src/main/j... google-api-client/src/main/java/com/google/api/client/googleapis/services/GoogleClient.java:99: return httpRequest; call setAllowEmptyContent()? http://codereview.appspot.com/5450097/diff/19003/google-api-client/src/main/j... google-api-client/src/main/java/com/google/api/client/googleapis/services/GoogleClient.java:115: methodOverride.intercept(httpRequest); call setAllowEmptyContent()? http://codereview.appspot.com/5450097/diff/23001/google-api-client/src/main/j... File google-api-client/src/main/java/com/google/api/client/googleapis/MediaExponentialBackOffPolicy.java (right): http://codereview.appspot.com/5450097/diff/23001/google-api-client/src/main/j... google-api-client/src/main/java/com/google/api/client/googleapis/MediaExponentialBackOffPolicy.java:37: protected MediaExponentialBackOffPolicy(MediaHttpUploader uploader) { package private http://codereview.appspot.com/5450097/diff/23001/google-api-client/src/main/j... google-api-client/src/main/java/com/google/api/client/googleapis/MediaExponentialBackOffPolicy.java:66: public MediaHttpUploader getUploader() { remove unused method? http://codereview.appspot.com/5450097/diff/23001/google-api-client/src/main/j... File google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java (right): http://codereview.appspot.com/5450097/diff/23001/google-api-client/src/main/j... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:60: /** Set before a media file chunk is uploaded. */ I'd rather this be sent after a media chunk has been uploaded. Right now the first MEDIA_IN_PROGRESS is sent immediately after INITIATION_COMPLETE which I think is not useful. Then after the last chunk has been sent, we should probably not send MEDIA_IN_PROGRESS but instead send MEDIA_COMPLETE. What do you think? http://codereview.appspot.com/5450097/diff/23001/google-api-client/src/main/j... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:352: public HttpRequestFactory getRequestFactory() { [optional] I'd recommend not exposing the HttpRequestFactory we use since it leaks some implementation detail that doesn't need to be leaked. I'd actually recommend doing the same for GoogleClient. I would like us to expose getHttpTransport(). http://codereview.appspot.com/5450097/diff/23001/google-api-client/src/main/j... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:476: if (bytesUploaded == 0) { bytesUploaded will be mediaContentLength on completion. So it should always be safe to just return the division result. http://codereview.appspot.com/5450097/diff/23001/google-api-client/src/main/j... File google-api-client/src/main/java/com/google/api/client/googleapis/services/GoogleClient.java (right): http://codereview.appspot.com/5450097/diff/23001/google-api-client/src/main/j... google-api-client/src/main/java/com/google/api/client/googleapis/services/GoogleClient.java:42: private final MethodOverride methodOverride = new MethodOverride(); Please change MethodOverride to use setAllowEmptyContent(false) as discussed in the other CL http://codereview.appspot.com/5450097/diff/23001/google-api-client/src/main/j... google-api-client/src/main/java/com/google/api/client/googleapis/services/GoogleClient.java:42: private final MethodOverride methodOverride = new MethodOverride(); It would be nice to avoid making a field out of this and just allocating it every time we need it. I think the performance penalty of allocating a local variable is negligible with modern JVMs. http://codereview.appspot.com/5450097/diff/23001/google-api-client/src/main/j... google-api-client/src/main/java/com/google/api/client/googleapis/services/GoogleClient.java:133: public static Builder builder(HttpTransport transport, JsonFactory jsonFactory, GenericUrl baseUrl) { /home/yanivi/hg/google-api-java-client/default/review/google-api-client/src/main/java/com/google/api/client/googleapis/services/GoogleClient.java:133: Line is longer than 100 characters. /home/yanivi/hg/google-api-java-client/default/review/google-api-client/src/main/java/com/google/api/client/googleapis/services/GoogleClient.java:187: Line is longer than 100 characters.
Sign in to reply to this message.
http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... File google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java (right): http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:124: Preconditions.checkArgument(this.mediaContentLength != -1); On 2012/01/26 21:44:50, yanivi wrote: > On 2012/01/20 21:46:21, yanivi wrote: > > lets move this precondition into getMediaContentLength() > > Ping Done. http://codereview.appspot.com/5450097/diff/4001/google-api-client/src/main/ja... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:207: byte[] block = new byte[blockSize]; On 2012/01/26 21:44:50, yanivi wrote: > On 2012/01/23 22:20:28, rmistry wrote: > > On 2012/01/20 21:46:21, yanivi wrote: > > > On 2012/01/19 16:20:18, rmistry wrote: > > > > On 2012/01/13 20:45:30, yanivi wrote: > > > > > In theory this use a lot of memory if chunkSize is large. Ideally we > > would > > > > want > > > > > to write directly from input stream to output. Essentially what we > > ideally > > > > want > > > > > is something like what's implemented in > > > > > AbstractInputStreamContent.write(OutputStream), except with the ability > to > > > > only > > > > > read a range of bytes from input stream. We could extract that > capability > > > > from > > > > > AbstractInputstreamContent into a utility method and call it from here. > > > > > > > > chunk size has been changed to an int, is this still a concern with > maximum > > > 2gb? > > > > Alternatively, we could enforce a non modifiable chunk size of 10mb. If > > > callers > > > > are not happy with the chunk size and would like to do a direct upload > they > > > > could call the code that will come in: > > > > http://code.google.com/p/google-api-java-client/issues/detail?id=391 > > > > > > Yes, I still think it is a potentially serious problem. Android developers > > have > > > complained about far smaller in-memory chunk sizes. > > > > Ok, I understand. Could you explain how the implementation would work? I > > understand how to write it to an OutputStream (using your example above with > > AbstractInputStreamContent.write) but do not see how to convert that into an > > HttpContent. > > I haven't looked at the details yet, but I'd think you should be able to just > use new InputStreamContent(contentInputStream) with setLength(blockSize). So > your implementation might actually be simpler with my proposal. The only > obvious catch I see is that AbstractInputStreamContent.writeTo actually closes > the input stream. We should probably add an option to > AbstractInputStreamContent disable closing the input stream (though it should be > enabled by default). Done, looks much better, thank you for the suggestion! AbstractInputStreamContent.writeTo will not close the input stream anymore in http://codereview.appspot.com/5572060/ so this will work (moved it to http://codereview.appspot.com/5482047/ as well) I also had to add a InputStreamContent.setRetrySupported to http://codereview.appspot.com/5482047/ to make this work. http://codereview.appspot.com/5450097/diff/19003/google-api-client/src/main/j... File google-api-client/src/main/java/com/google/api/client/googleapis/services/GoogleClient.java (right): http://codereview.appspot.com/5450097/diff/19003/google-api-client/src/main/j... google-api-client/src/main/java/com/google/api/client/googleapis/services/GoogleClient.java:99: return httpRequest; On 2012/01/26 21:44:50, yanivi wrote: > call setAllowEmptyContent()? Not needed because methodOverride is now doing this? http://codereview.appspot.com/5450097/diff/19003/google-api-client/src/main/j... google-api-client/src/main/java/com/google/api/client/googleapis/services/GoogleClient.java:115: methodOverride.intercept(httpRequest); On 2012/01/26 21:44:50, yanivi wrote: > call setAllowEmptyContent()? Not needed because methodOverride is now doing this? http://codereview.appspot.com/5450097/diff/23001/google-api-client/src/main/j... File google-api-client/src/main/java/com/google/api/client/googleapis/MediaExponentialBackOffPolicy.java (right): http://codereview.appspot.com/5450097/diff/23001/google-api-client/src/main/j... google-api-client/src/main/java/com/google/api/client/googleapis/MediaExponentialBackOffPolicy.java:37: protected MediaExponentialBackOffPolicy(MediaHttpUploader uploader) { On 2012/01/26 21:44:51, yanivi wrote: > package private Done. http://codereview.appspot.com/5450097/diff/23001/google-api-client/src/main/j... google-api-client/src/main/java/com/google/api/client/googleapis/MediaExponentialBackOffPolicy.java:66: public MediaHttpUploader getUploader() { On 2012/01/26 21:44:51, yanivi wrote: > remove unused method? Done. http://codereview.appspot.com/5450097/diff/23001/google-api-client/src/main/j... File google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java (right): http://codereview.appspot.com/5450097/diff/23001/google-api-client/src/main/j... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:60: /** Set before a media file chunk is uploaded. */ On 2012/01/26 21:44:51, yanivi wrote: > I'd rather this be sent after a media chunk has been uploaded. Right now the > first MEDIA_IN_PROGRESS is sent immediately after INITIATION_COMPLETE which I > think is not useful. Then after the last chunk has been sent, we should > probably not send MEDIA_IN_PROGRESS but instead send MEDIA_COMPLETE. What do > you think? I went back and forth on when the statuses should be set, your proposal sgtm. http://codereview.appspot.com/5450097/diff/23001/google-api-client/src/main/j... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:352: public HttpRequestFactory getRequestFactory() { On 2012/01/26 21:44:51, yanivi wrote: > [optional] I'd recommend not exposing the HttpRequestFactory we use since it > leaks some implementation detail that doesn't need to be leaked. I'd actually > recommend doing the same for GoogleClient. I would like us to expose > getHttpTransport(). Done in this class. I will do GoogleClient in a later CL. http://codereview.appspot.com/5450097/diff/23001/google-api-client/src/main/j... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:476: if (bytesUploaded == 0) { On 2012/01/26 21:44:51, yanivi wrote: > bytesUploaded will be mediaContentLength on completion. So it should always be > safe to just return the division result. Done. http://codereview.appspot.com/5450097/diff/23001/google-api-client/src/main/j... File google-api-client/src/main/java/com/google/api/client/googleapis/services/GoogleClient.java (right): http://codereview.appspot.com/5450097/diff/23001/google-api-client/src/main/j... google-api-client/src/main/java/com/google/api/client/googleapis/services/GoogleClient.java:42: private final MethodOverride methodOverride = new MethodOverride(); On 2012/01/26 21:44:51, yanivi wrote: > It would be nice to avoid making a field out of this and just allocating it > every time we need it. I think the performance penalty of allocating a local > variable is negligible with modern JVMs. Done. http://codereview.appspot.com/5450097/diff/23001/google-api-client/src/main/j... google-api-client/src/main/java/com/google/api/client/googleapis/services/GoogleClient.java:133: public static Builder builder(HttpTransport transport, JsonFactory jsonFactory, GenericUrl baseUrl) { On 2012/01/26 21:44:51, yanivi wrote: > /home/yanivi/hg/google-api-java-client/default/review/google-api-client/src/main/java/com/google/api/client/googleapis/services/GoogleClient.java:133: > Line is longer than 100 characters. > /home/yanivi/hg/google-api-java-client/default/review/google-api-client/src/main/java/com/google/api/client/googleapis/services/GoogleClient.java:187: > Line is longer than 100 characters. Done.
Sign in to reply to this message.
just one naming issue http://codereview.appspot.com/5450097/diff/34001/google-api-client/src/main/j... File google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java (right): http://codereview.appspot.com/5450097/diff/34001/google-api-client/src/main/j... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:222: Preconditions.checkArgument(mediaContentLength != -1); nitpick: move this inside the if-statement above. http://codereview.appspot.com/5450097/diff/34001/google-api-client/src/main/j... File google-api-client/src/main/java/com/google/api/client/googleapis/MethodOverride.java (right): http://codereview.appspot.com/5450097/diff/34001/google-api-client/src/main/j... google-api-client/src/main/java/com/google/api/client/googleapis/MethodOverride.java:78: public void intercept(HttpRequest request) throws IOException { Description Resource Path Location Type The declared exception IOException is not actually thrown by the method intercept(HttpRequest) from type MethodOverride MethodOverride.java /google-api-client/src/main/java/com/google/api/client/googleapis line 78 Java Problem http://codereview.appspot.com/5450097/diff/34001/google-api-client/src/main/j... File google-api-client/src/main/java/com/google/api/client/googleapis/ProgressListener.java (right): http://codereview.appspot.com/5450097/diff/34001/google-api-client/src/main/j... google-api-client/src/main/java/com/google/api/client/googleapis/ProgressListener.java:51: public interface ProgressListener { I don't like the naming here because it is too generic. I would suggest either: 1. move ProgressListener as an inner class to MediaHttpUploader, since that's the only context where this progress listener is valid 2. rename it to something like generic like MediaHttpUploaderProgressListener
Sign in to reply to this message.
http://codereview.appspot.com/5450097/diff/34001/google-api-client/src/main/j... File google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java (right): http://codereview.appspot.com/5450097/diff/34001/google-api-client/src/main/j... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:222: Preconditions.checkArgument(mediaContentLength != -1); On 2012/01/27 17:08:51, yanivi wrote: > nitpick: move this inside the if-statement above. Done. http://codereview.appspot.com/5450097/diff/34001/google-api-client/src/main/j... File google-api-client/src/main/java/com/google/api/client/googleapis/MethodOverride.java (right): http://codereview.appspot.com/5450097/diff/34001/google-api-client/src/main/j... google-api-client/src/main/java/com/google/api/client/googleapis/MethodOverride.java:78: public void intercept(HttpRequest request) throws IOException { On 2012/01/27 17:08:51, yanivi wrote: > Description Resource Path Location Type > The declared exception IOException is not actually thrown by the method > intercept(HttpRequest) from type > MethodOverride MethodOverride.java /google-api-client/src/main/java/com/google/api/client/googleapis line > 78 Java Problem Done. http://codereview.appspot.com/5450097/diff/34001/google-api-client/src/main/j... File google-api-client/src/main/java/com/google/api/client/googleapis/ProgressListener.java (right): http://codereview.appspot.com/5450097/diff/34001/google-api-client/src/main/j... google-api-client/src/main/java/com/google/api/client/googleapis/ProgressListener.java:51: public interface ProgressListener { On 2012/01/27 17:08:51, yanivi wrote: > I don't like the naming here because it is too generic. I would suggest either: > > 1. move ProgressListener as an inner class to MediaHttpUploader, since that's > the only context where this progress listener is valid > > 2. rename it to something like generic like MediaHttpUploaderProgressListener Renamed to MediaHttpUploaderProgressListener
Sign in to reply to this message.
http://codereview.appspot.com/5450097/diff/37002/google-api-client/src/main/j... File google-api-client/src/main/java/com/google/api/client/googleapis/services/GoogleClient.java (right): http://codereview.appspot.com/5450097/diff/37002/google-api-client/src/main/j... google-api-client/src/main/java/com/google/api/client/googleapis/services/GoogleClient.java:1: /* please do an hg pull -u I'm getting a conflict on this file
Sign in to reply to this message.
http://codereview.appspot.com/5450097/diff/37002/google-api-client/src/main/j... File google-api-client/src/main/java/com/google/api/client/googleapis/services/GoogleClient.java (right): http://codereview.appspot.com/5450097/diff/37002/google-api-client/src/main/j... google-api-client/src/main/java/com/google/api/client/googleapis/services/GoogleClient.java:1: /* On 2012/01/28 13:04:36, yanivi wrote: > please do an hg pull -u > I'm getting a conflict on this file Done.
Sign in to reply to this message.
Ping.
Sign in to reply to this message.
http://codereview.appspot.com/5450097/diff/28012/google-api-client/src/main/j... File google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java (right): http://codereview.appspot.com/5450097/diff/28012/google-api-client/src/main/j... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:398: Preconditions.checkArgument(chunkSize >= 1); please change this to chunkSize >= 256KB
Sign in to reply to this message.
http://codereview.appspot.com/5450097/diff/28012/google-api-client/src/main/j... File google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java (right): http://codereview.appspot.com/5450097/diff/28012/google-api-client/src/main/j... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:398: Preconditions.checkArgument(chunkSize >= 1); On 2012/02/07 18:09:39, yanivi wrote: > please change this to chunkSize >= 256KB Done.
Sign in to reply to this message.
Synced with head.
Sign in to reply to this message.
http://codereview.appspot.com/5450097/diff/45005/google-api-client/src/main/j... File google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java (right): http://codereview.appspot.com/5450097/diff/45005/google-api-client/src/main/j... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:200: response = e.getResponse(); Description Resource Path Location Type The method getResponse() from the type HttpResponseException is deprecated MediaHttpUploader.java /google-api-client/src/main/java/com/google/api/client/googleapis line 200 Java Problem The method getResponse() from the type HttpResponseException is deprecated MediaHttpUploader.java /google-api-client/src/main/java/com/google/api/client/googleapis line 298 Java Problem http://codereview.appspot.com/5450097/diff/45005/google-api-client/src/main/j... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:201: // TODO(rmistry): Use HttpRequest.setThrowExceptionOnExecuteError(false) when it is please fix now http://codereview.appspot.com/5450097/diff/45005/google-api-client/src/main/j... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:204: throw e; should probably use GoogleJsonResponseException.from here http://codereview.appspot.com/5450097/diff/45005/google-api-client/src/main/j... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:398: public MediaHttpUploader setChunkSize(int chunkSize) { JavaDoc should specify minimum chunk size. http://codereview.appspot.com/5450097/diff/45005/google-api-client/src/main/j... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:399: Preconditions.checkArgument(chunkSize >= 256 * KB); please make a constant for the minimum chunk size http://codereview.appspot.com/5450097/diff/45005/google-api-client/src/main/j... File google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploaderProgressListener.java (right): http://codereview.appspot.com/5450097/diff/45005/google-api-client/src/main/j... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploaderProgressListener.java:39: System.out.println("Upload percentage: " + uploader.getProgress()); Description Resource Path Location Type Unhandled exception type IOException MediaHttpUploaderProgressListener.java /google-api-client/src/main/java/com/google/api/client/googleapis line 64 Java Problem solve this by adding "throws IOException" to progressChanged on line 29
Sign in to reply to this message.
http://codereview.appspot.com/5450097/diff/45005/google-api-client/src/main/j... File google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java (right): http://codereview.appspot.com/5450097/diff/45005/google-api-client/src/main/j... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:178: contentInputStream = mediaContent.newInputStream(); Description Resource Path Location Type The method newInputStream() is undefined for the type AbstractInputStreamContent MediaHttpUploader.java /google-api-client/src/main/java/com/google/api/client/googleapis line 178 Java Problem
Sign in to reply to this message.
http://codereview.appspot.com/5450097/diff/45005/google-api-client/src/main/j... File google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java (right): http://codereview.appspot.com/5450097/diff/45005/google-api-client/src/main/j... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:178: contentInputStream = mediaContent.newInputStream(); On 2012/02/09 15:22:17, yanivi wrote: > Description Resource Path Location Type > The method newInputStream() is undefined for the type > AbstractInputStreamContent MediaHttpUploader.java /google-api-client/src/main/java/com/google/api/client/googleapis line > 178 Java Problem Done. http://codereview.appspot.com/5450097/diff/45005/google-api-client/src/main/j... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:200: response = e.getResponse(); On 2012/02/08 18:25:05, yanivi wrote: > Description Resource Path Location Type > The method getResponse() from the type HttpResponseException is > deprecated MediaHttpUploader.java /google-api-client/src/main/java/com/google/api/client/googleapis line > 200 Java Problem > The method getResponse() from the type HttpResponseException is > deprecated MediaHttpUploader.java /google-api-client/src/main/java/com/google/api/client/googleapis line > 298 Java Problem Done. http://codereview.appspot.com/5450097/diff/45005/google-api-client/src/main/j... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:201: // TODO(rmistry): Use HttpRequest.setThrowExceptionOnExecuteError(false) when it is On 2012/02/08 18:25:05, yanivi wrote: > please fix now Done. http://codereview.appspot.com/5450097/diff/45005/google-api-client/src/main/j... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:204: throw e; On 2012/02/08 18:25:05, yanivi wrote: > should probably use GoogleJsonResponseException.from here Done. http://codereview.appspot.com/5450097/diff/45005/google-api-client/src/main/j... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:398: public MediaHttpUploader setChunkSize(int chunkSize) { On 2012/02/08 18:25:05, yanivi wrote: > JavaDoc should specify minimum chunk size. Done. http://codereview.appspot.com/5450097/diff/45005/google-api-client/src/main/j... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:399: Preconditions.checkArgument(chunkSize >= 256 * KB); On 2012/02/08 18:25:05, yanivi wrote: > please make a constant for the minimum chunk size Done. http://codereview.appspot.com/5450097/diff/45005/google-api-client/src/main/j... File google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploaderProgressListener.java (right): http://codereview.appspot.com/5450097/diff/45005/google-api-client/src/main/j... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploaderProgressListener.java:39: System.out.println("Upload percentage: " + uploader.getProgress()); On 2012/02/08 18:25:05, yanivi wrote: > Description Resource Path Location Type > Unhandled exception type > IOException MediaHttpUploaderProgressListener.java /google-api-client/src/main/java/com/google/api/client/googleapis line > 64 Java Problem > > > solve this by adding "throws IOException" to progressChanged on line 29 Done.
Sign in to reply to this message.
LGTM http://codereview.appspot.com/5450097/diff/51002/google-api-client/src/main/j... File google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java (right): http://codereview.appspot.com/5450097/diff/51002/google-api-client/src/main/j... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:76: * Mininum number of bytes that can be uploaded to the server (set to 256KB). mininum -> minimum
Sign in to reply to this message.
http://codereview.appspot.com/5450097/diff/51002/google-api-client/src/main/j... File google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java (right): http://codereview.appspot.com/5450097/diff/51002/google-api-client/src/main/j... google-api-client/src/main/java/com/google/api/client/googleapis/MediaHttpUploader.java:76: * Mininum number of bytes that can be uploaded to the server (set to 256KB). On 2012/02/09 21:53:39, yanivi wrote: > mininum -> minimum Done.
Sign in to reply to this message.
|
