|
|
DescriptionFix a bug in ResumableUpload when media size is unknown
Patch Set 1 #Patch Set 2 : Fix the bug #
Total comments: 21
Patch Set 3 : There is nothing better than self review :) #Patch Set 4 : minor #Patch Set 5 : minor #Patch Set 6 : minor #Patch Set 7 : minor #
Total comments: 14
Patch Set 8 : Class review #
Total comments: 4
Patch Set 9 : typo #
MessagesTotal messages: 9
https://codereview.appspot.com/11347044/diff/3001/Src/GoogleApis.Tests/Apis/U... File Src/GoogleApis.Tests/Apis/Upload/ResumableUploadTest.cs (left): https://codereview.appspot.com/11347044/diff/3001/Src/GoogleApis.Tests/Apis/U... Src/GoogleApis.Tests/Apis/Upload/ResumableUploadTest.cs:609: public void TestChunkUpload_ServerUnavailable_KnownSize() Add comment. This test covered the all 3 possibilities - server doesn't read bytes on error, server read partial bytes on error, server reads all bytes but somehow return 503 https://codereview.appspot.com/11347044/diff/3001/Src/GoogleApis.Tests/Apis/U... File Src/GoogleApis.Tests/Apis/Upload/ResumableUploadTest.cs (right): https://codereview.appspot.com/11347044/diff/3001/Src/GoogleApis.Tests/Apis/U... Src/GoogleApis.Tests/Apis/Upload/ResumableUploadTest.cs:55: ApplicationContext.RegisterLogger(new Google.Apis.Logging.Log4NetLogger()); leave it comment https://codereview.appspot.com/11347044/diff/3001/Src/GoogleApis.Tests/Apis/U... Src/GoogleApis.Tests/Apis/Upload/ResumableUploadTest.cs:307: /// <summary> Gets or sets the amount of bytes there server reads on error. </summary> there -> the mount of bytes the server reads when error occurred. The default is 0, meaning that on error the server won't read any bytes https://codereview.appspot.com/11347044/diff/3001/Src/GoogleApis.Tests/Apis/U... Src/GoogleApis.Tests/Apis/Upload/ResumableUploadTest.cs:646: SubtestTestChunkUpload(true, 13, ServerError.ServerUnavailable, 50, 1); add also tests for last chunk failed (e.g. chunk size = 300) https://codereview.appspot.com/11347044/diff/3001/Src/GoogleApis.Tests/Apis/U... Src/GoogleApis.Tests/Apis/Upload/ResumableUploadTest.cs:695: // we expect 4 calls: 1 initial request + 3 chunks (0-99, 100-199, 200-299), then the client receive 4xx then -> on the last chunk https://codereview.appspot.com/11347044/diff/3001/Src/GoogleApis.Tests/Apis/U... Src/GoogleApis.Tests/Apis/Upload/ResumableUploadTest.cs:701: int chunkSize = 100, int readBytesOnError = 0) change the setter in ResumableUpload to check chunk size, and make the field internal [VisibleToTestOnly] so we would be able to test it with "testable" chunks https://codereview.appspot.com/11347044/diff/3001/Src/GoogleApis/Apis/[Media]... File Src/GoogleApis/Apis/[Media]/Upload/ResumableUpload.cs (left): https://codereview.appspot.com/11347044/diff/3001/Src/GoogleApis/Apis/[Media]... Src/GoogleApis/Apis/[Media]/Upload/ResumableUpload.cs:338: private async Task<IUploadProgress> Upload(CancellationToken cancellationToken) add TODO(peleyal): should we add a resume method? https://codereview.appspot.com/11347044/diff/3001/Src/GoogleApis/Apis/[Media]... File Src/GoogleApis/Apis/[Media]/Upload/ResumableUpload.cs (right): https://codereview.appspot.com/11347044/diff/3001/Src/GoogleApis/Apis/[Media]... Src/GoogleApis/Apis/[Media]/Upload/ResumableUpload.cs:167: /// <summary> Gets and sets the amount of bytes the server had received so far. </summary> or https://codereview.appspot.com/11347044/diff/3001/Src/GoogleApis/Apis/[Media]... Src/GoogleApis/Apis/[Media]/Upload/ResumableUpload.cs:170: /// <summary> Gets and sets the amount of bytes the client had sent so far. </summary> or https://codereview.appspot.com/11347044/diff/3001/Src/GoogleApis/Apis/[Media]... Src/GoogleApis/Apis/[Media]/Upload/ResumableUpload.cs:499: // that we need to resend bytes from the last request resend (and copy to the current request's stream) https://codereview.appspot.com/11347044/diff/3001/Src/GoogleApis/Apis/[Media]... Src/GoogleApis/Apis/[Media]/Upload/ResumableUpload.cs:565: // that we need to change the position of the input stream , otherwise we continue to send bytes form the current position
Sign in to reply to this message.
https://codereview.appspot.com/11347044/diff/3001/Src/GoogleApis.Tests/Apis/U... File Src/GoogleApis.Tests/Apis/Upload/ResumableUploadTest.cs (left): https://codereview.appspot.com/11347044/diff/3001/Src/GoogleApis.Tests/Apis/U... Src/GoogleApis.Tests/Apis/Upload/ResumableUploadTest.cs:609: public void TestChunkUpload_ServerUnavailable_KnownSize() On 2013/07/22 01:02:58, peleyal wrote: > Add comment. This test covered the all 3 possibilities - server doesn't read > bytes on error, server read partial bytes on error, server reads all bytes but > somehow return 503 Done. https://codereview.appspot.com/11347044/diff/3001/Src/GoogleApis.Tests/Apis/U... File Src/GoogleApis.Tests/Apis/Upload/ResumableUploadTest.cs (right): https://codereview.appspot.com/11347044/diff/3001/Src/GoogleApis.Tests/Apis/U... Src/GoogleApis.Tests/Apis/Upload/ResumableUploadTest.cs:55: ApplicationContext.RegisterLogger(new Google.Apis.Logging.Log4NetLogger()); On 2013/07/22 01:02:58, peleyal wrote: > leave it comment Done. https://codereview.appspot.com/11347044/diff/3001/Src/GoogleApis.Tests/Apis/U... Src/GoogleApis.Tests/Apis/Upload/ResumableUploadTest.cs:307: /// <summary> Gets or sets the amount of bytes there server reads on error. </summary> On 2013/07/22 01:02:58, peleyal wrote: > there -> the > mount of bytes the server reads when error occurred. The default is 0, meaning > that on error the server won't read any bytes Done. https://codereview.appspot.com/11347044/diff/3001/Src/GoogleApis.Tests/Apis/U... Src/GoogleApis.Tests/Apis/Upload/ResumableUploadTest.cs:646: SubtestTestChunkUpload(true, 13, ServerError.ServerUnavailable, 50, 1); On 2013/07/22 01:02:58, peleyal wrote: > add also tests for last chunk failed (e.g. chunk size = 300) Done. https://codereview.appspot.com/11347044/diff/3001/Src/GoogleApis.Tests/Apis/U... Src/GoogleApis.Tests/Apis/Upload/ResumableUploadTest.cs:695: // we expect 4 calls: 1 initial request + 3 chunks (0-99, 100-199, 200-299), then the client receive 4xx On 2013/07/22 01:02:58, peleyal wrote: > then -> on the last chunk Done. https://codereview.appspot.com/11347044/diff/3001/Src/GoogleApis.Tests/Apis/U... Src/GoogleApis.Tests/Apis/Upload/ResumableUploadTest.cs:701: int chunkSize = 100, int readBytesOnError = 0) On 2013/07/22 01:02:58, peleyal wrote: > change the setter in ResumableUpload to check chunk size, and make the field > internal [VisibleToTestOnly] so we would be able to test it with "testable" > chunks Done. https://codereview.appspot.com/11347044/diff/3001/Src/GoogleApis/Apis/[Media]... File Src/GoogleApis/Apis/[Media]/Upload/ResumableUpload.cs (right): https://codereview.appspot.com/11347044/diff/3001/Src/GoogleApis/Apis/[Media]... Src/GoogleApis/Apis/[Media]/Upload/ResumableUpload.cs:167: /// <summary> Gets and sets the amount of bytes the server had received so far. </summary> On 2013/07/22 01:02:58, peleyal wrote: > or Done. https://codereview.appspot.com/11347044/diff/3001/Src/GoogleApis/Apis/[Media]... Src/GoogleApis/Apis/[Media]/Upload/ResumableUpload.cs:170: /// <summary> Gets and sets the amount of bytes the client had sent so far. </summary> On 2013/07/22 01:02:58, peleyal wrote: > or Done. https://codereview.appspot.com/11347044/diff/3001/Src/GoogleApis/Apis/[Media]... Src/GoogleApis/Apis/[Media]/Upload/ResumableUpload.cs:499: // that we need to resend bytes from the last request On 2013/07/22 01:02:58, peleyal wrote: > resend (and copy to the current request's stream) Done. https://codereview.appspot.com/11347044/diff/3001/Src/GoogleApis/Apis/[Media]... Src/GoogleApis/Apis/[Media]/Upload/ResumableUpload.cs:565: // that we need to change the position of the input stream On 2013/07/22 01:02:58, peleyal wrote: > , otherwise we continue to send bytes form the current position Done.
Sign in to reply to this message.
Hi Gus, Like I promised attached is a fix to resumable media upload. I fixed it in the Java client library and I was pretty sure that we are going to have a similar bug in the .NET client library. So I added more tests, and I'm happy with current implementation. Thanks, Eyal
Sign in to reply to this message.
On 2013/07/23 22:05:10, peleyal wrote: > Hi Gus, > Like I promised attached is a fix to resumable media upload. > I fixed it in the Java client library and I was pretty sure that we are going to > have a similar bug in the .NET client library. > > So I added more tests, and I'm happy with current implementation. > > Thanks, > Eyal Ping ... :)
Sign in to reply to this message.
A few documentation comments and a design consideration https://codereview.appspot.com/11347044/diff/20006/Src/GoogleApis.Tests/Apis/... File Src/GoogleApis.Tests/Apis/Upload/ResumableUploadTest.cs (right): https://codereview.appspot.com/11347044/diff/20006/Src/GoogleApis.Tests/Apis/... Src/GoogleApis.Tests/Apis/Upload/ResumableUploadTest.cs:52: public void SetUp() Consider passing a boolean instead of commenting out https://codereview.appspot.com/11347044/diff/20006/Src/GoogleApis.Tests/Apis/... Src/GoogleApis.Tests/Apis/Upload/ResumableUploadTest.cs:308: /// Gets or sets the amount of bytes the server reads when error occurred. The default value is <c>0</c>, Gets or sets the *number* of bytes https://codereview.appspot.com/11347044/diff/20006/Src/GoogleApis.Tests/Apis/... Src/GoogleApis.Tests/Apis/Upload/ResumableUploadTest.cs:654: /// An helper test which tests that the client accepts 308 and 503 responses when uploading chunks. This test This should be "A helper test that tests..." https://codereview.appspot.com/11347044/diff/20006/Src/GoogleApis.Tests/Apis/... Src/GoogleApis.Tests/Apis/Upload/ResumableUploadTest.cs:1039: // less that the minimum Less than the minimum? https://codereview.appspot.com/11347044/diff/20006/Src/GoogleApis/Apis/[Media... File Src/GoogleApis/Apis/[Media]/Upload/ResumableUpload.cs (right): https://codereview.appspot.com/11347044/diff/20006/Src/GoogleApis/Apis/[Media... Src/GoogleApis/Apis/[Media]/Upload/ResumableUpload.cs:178: /// Gets or sets the size of each chunk sent to the server. I don't have too strong of an opinion on this but I think it would be acceptable to remove the language pattern: "Gets or sets the ..." We could alternatively use something like: "The size of each chunk sent to the server." https://codereview.appspot.com/11347044/diff/20006/Src/GoogleApis/Apis/[Media... Src/GoogleApis/Apis/[Media]/Upload/ResumableUpload.cs:510: // if the number of bytes received by the sever isn't equal to the amount of bytes the client sent, it A revision: // if the number of bytes received by the server isn't equal to the amount of bytes the client sent, // resend bytes and copy to the current request's stream from the last request server was misspelled and the last part of the comment was made a little more concise
Sign in to reply to this message.
https://codereview.appspot.com/11347044/diff/20006/Src/GoogleApis.Tests/Apis/... File Src/GoogleApis.Tests/Apis/Upload/ResumableUploadTest.cs (right): https://codereview.appspot.com/11347044/diff/20006/Src/GoogleApis.Tests/Apis/... Src/GoogleApis.Tests/Apis/Upload/ResumableUploadTest.cs:52: public void SetUp() On 2013/08/02 22:01:20, class wrote: > Consider passing a boolean instead of commenting out Done. https://codereview.appspot.com/11347044/diff/20006/Src/GoogleApis.Tests/Apis/... Src/GoogleApis.Tests/Apis/Upload/ResumableUploadTest.cs:308: /// Gets or sets the amount of bytes the server reads when error occurred. The default value is <c>0</c>, On 2013/08/02 22:01:20, class wrote: > Gets or sets the *number* of bytes Done. https://codereview.appspot.com/11347044/diff/20006/Src/GoogleApis.Tests/Apis/... Src/GoogleApis.Tests/Apis/Upload/ResumableUploadTest.cs:654: /// An helper test which tests that the client accepts 308 and 503 responses when uploading chunks. This test On 2013/08/02 22:01:20, class wrote: > This should be "A helper test that tests..." Done. https://codereview.appspot.com/11347044/diff/20006/Src/GoogleApis.Tests/Apis/... Src/GoogleApis.Tests/Apis/Upload/ResumableUploadTest.cs:1039: // less that the minimum On 2013/08/02 22:01:20, class wrote: > Less than the minimum? Done. https://codereview.appspot.com/11347044/diff/20006/Src/GoogleApis/Apis/[Media... File Src/GoogleApis/Apis/[Media]/Upload/ResumableUpload.cs (right): https://codereview.appspot.com/11347044/diff/20006/Src/GoogleApis/Apis/[Media... Src/GoogleApis/Apis/[Media]/Upload/ResumableUpload.cs:178: /// Gets or sets the size of each chunk sent to the server. I prefer the 'gets or sets' just because it's the way the framework classes were documented. On 2013/08/02 22:01:20, class wrote: > I don't have too strong of an opinion on this but I think it would be acceptable > to remove the language pattern: > > "Gets or sets the ..." > > We could alternatively use something like: > "The size of each chunk sent to the server." https://codereview.appspot.com/11347044/diff/20006/Src/GoogleApis/Apis/[Media... Src/GoogleApis/Apis/[Media]/Upload/ResumableUpload.cs:510: // if the number of bytes received by the sever isn't equal to the amount of bytes the client sent, it On 2013/08/02 22:01:20, class wrote: > A revision: > > // if the number of bytes received by the server isn't equal to the amount of > bytes the client sent, > // resend bytes and copy to the current request's stream from the last request > > server was misspelled and the last part of the comment was made a little more > concise Done.
Sign in to reply to this message.
There is still one typo I missed the first time, LGTM after renaming. https://codereview.appspot.com/11347044/diff/20006/Src/GoogleApis.Tests/Apis/... File Src/GoogleApis.Tests/Apis/Upload/ResumableUploadTest.cs (right): https://codereview.appspot.com/11347044/diff/20006/Src/GoogleApis.Tests/Apis/... Src/GoogleApis.Tests/Apis/Upload/ResumableUploadTest.cs:424: if (bytesRecieved != len) Minor spelling nit: bytesReceieved https://codereview.appspot.com/11347044/diff/38001/Src/GoogleApis.Tests/Apis/... File Src/GoogleApis.Tests/Apis/Upload/ResumableUploadTest.cs (right): https://codereview.appspot.com/11347044/diff/38001/Src/GoogleApis.Tests/Apis/... Src/GoogleApis.Tests/Apis/Upload/ResumableUploadTest.cs:328: private int bytesRecieved = 0; Missed this the first time, this should be bytesReceived, minor typo https://codereview.appspot.com/11347044/diff/38001/Src/GoogleApis.Tests/Apis/... Src/GoogleApis.Tests/Apis/Upload/ResumableUploadTest.cs:445: if (bytesRecieved != len) (as above) refactor to bytesReceived
Sign in to reply to this message.
My bad. Waiting for your approval. Thanks, Eyal https://codereview.appspot.com/11347044/diff/20006/Src/GoogleApis.Tests/Apis/... File Src/GoogleApis.Tests/Apis/Upload/ResumableUploadTest.cs (right): https://codereview.appspot.com/11347044/diff/20006/Src/GoogleApis.Tests/Apis/... Src/GoogleApis.Tests/Apis/Upload/ResumableUploadTest.cs:424: if (bytesRecieved != len) On 2013/08/08 17:32:31, class wrote: > Minor spelling nit: bytesReceieved Done. https://codereview.appspot.com/11347044/diff/38001/Src/GoogleApis.Tests/Apis/... File Src/GoogleApis.Tests/Apis/Upload/ResumableUploadTest.cs (right): https://codereview.appspot.com/11347044/diff/38001/Src/GoogleApis.Tests/Apis/... Src/GoogleApis.Tests/Apis/Upload/ResumableUploadTest.cs:328: private int bytesRecieved = 0; On 2013/08/08 17:32:31, class wrote: > Missed this the first time, this should be bytesReceived, minor typo Done. https://codereview.appspot.com/11347044/diff/38001/Src/GoogleApis.Tests/Apis/... Src/GoogleApis.Tests/Apis/Upload/ResumableUploadTest.cs:445: if (bytesRecieved != len) On 2013/08/08 17:32:31, class wrote: > (as above) refactor to bytesReceived Done.
Sign in to reply to this message.
|