Code review - Issue 13480044: Issue 361: MediaDownloader can't download drive export list (which includes query parameters)https://codereview.appspot.com/2013-10-14T19:27:10+00:00rietveld
Message from unknown
2013-09-14T01:21:07+00:00peleyalurn:md5:0dd4d1712419ff668c51f745974646eb
Message from peleyal@google.com
2013-09-14T01:22:49+00:00peleyalurn:md5:ae9d97461061214494b63f1f3864d2be
Message from unknown
2013-09-14T01:46:59+00:00peleyalurn:md5:7004bcfcb16b470630c64c76dc244783
Message from unknown
2013-09-14T01:50:03+00:00peleyalurn:md5:94ee75684b74cf6b7f4e74e8ba9ff8fe
Message from unknown
2013-09-14T01:50:48+00:00peleyalurn:md5:5ec4ffaa4e8eb52592f3dea59f24d11f
Message from ngmiceli@google.com
2013-10-04T20:12:22+00:00ngmiceliurn:md5:3b97832a2cf4db3959a735232b7728a9
https://codereview.appspot.com/13480044/diff/12001/Src/GoogleApis/Apis/Http/ConfigurableMessageHandler.cs
File Src/GoogleApis/Apis/Http/ConfigurableMessageHandler.cs (right):
https://codereview.appspot.com/13480044/diff/12001/Src/GoogleApis/Apis/Http/ConfigurableMessageHandler.cs#newcode97
Src/GoogleApis/Apis/Http/ConfigurableMessageHandler.cs:97: /// only the allowed tries of >=400 responses, or in a case an exception was thrown.
This is fine on a technical basis, but it's a little confusing to the user if they haven't read the source code. What happens if the numTries = 0, but numRedirects = 10? While I know that means it won't retry OR redirect, the user may expect that means ONLY handle redirects, don't do any other retries. The interactions between the two are not well explained here.
https://codereview.appspot.com/13480044/diff/12001/Src/GoogleApis/Apis/[Media]/Download/MediaDownloader.cs
File Src/GoogleApis/Apis/[Media]/Download/MediaDownloader.cs (right):
https://codereview.appspot.com/13480044/diff/12001/Src/GoogleApis/Apis/[Media]/Download/MediaDownloader.cs#newcode205
Src/GoogleApis/Apis/[Media]/Download/MediaDownloader.cs:205: var pairs = from parameter in query.Split('&')
This block of code is rather generic. Are you sure there isn't a built in method to do this? If not, perhaps you could make it a utility method?
https://codereview.appspot.com/13480044/diff/12001/Src/GoogleApis/Apis/[Media]/Download/MediaDownloader.cs#newcode247
Src/GoogleApis/Apis/[Media]/Download/MediaDownloader.cs:247: // content range is null in case the server doesn't adhere the media download protocol, in
"in the case" -> "when"
https://codereview.appspot.com/13480044/diff/12001/Src/GoogleApis/Apis/[Media]/Download/MediaDownloader.cs#newcode249
Src/GoogleApis/Apis/[Media]/Download/MediaDownloader.cs:249: currentRequestFirstBytePos = mediaContentLength =
Is this logic tested?
Message from unknown
2013-10-10T20:03:21+00:00peleyalurn:md5:7a1f9e7bf0b37603a02054b448bd5e20
Message from unknown
2013-10-10T20:06:21+00:00peleyalurn:md5:3b8b06838deeb85766079cb70680952c
Message from peleyal@google.com
2013-10-10T20:06:37+00:00peleyalurn:md5:5cd49437445a4ef7ec26e4dd6e4a7e26
https://codereview.appspot.com/13480044/diff/12001/Src/GoogleApis/Apis/Http/ConfigurableMessageHandler.cs
File Src/GoogleApis/Apis/Http/ConfigurableMessageHandler.cs (right):
https://codereview.appspot.com/13480044/diff/12001/Src/GoogleApis/Apis/Http/ConfigurableMessageHandler.cs#newcode97
Src/GoogleApis/Apis/Http/ConfigurableMessageHandler.cs:97: /// only the allowed tries of >=400 responses, or in a case an exception was thrown.
numTries can't be 0. it should be more than 1.
so if numTries = 1 and numRedirect = 10, I think it's obvious, am I right?
On 2013/10/04 20:12:23, ngmiceli wrote:
> This is fine on a technical basis, but it's a little confusing to the user if
> they haven't read the source code. What happens if the numTries = 0, but
> numRedirects = 10? While I know that means it won't retry OR redirect, the user
> may expect that means ONLY handle redirects, don't do any other retries. The
> interactions between the two are not well explained here.
https://codereview.appspot.com/13480044/diff/12001/Src/GoogleApis/Apis/[Media]/Download/MediaDownloader.cs
File Src/GoogleApis/Apis/[Media]/Download/MediaDownloader.cs (right):
https://codereview.appspot.com/13480044/diff/12001/Src/GoogleApis/Apis/[Media]/Download/MediaDownloader.cs#newcode205
Src/GoogleApis/Apis/[Media]/Download/MediaDownloader.cs:205: var pairs = from parameter in query.Split('&')
ParseQueryString is not PCL!
I don't need that in any other place, so I don't think to add a utility now, unless it's important for you, then I'll do that :)
On 2013/10/04 20:12:23, ngmiceli wrote:
> This block of code is rather generic. Are you sure there isn't a built in method
> to do this? If not, perhaps you could make it a utility method?
https://codereview.appspot.com/13480044/diff/12001/Src/GoogleApis/Apis/[Media]/Download/MediaDownloader.cs#newcode247
Src/GoogleApis/Apis/[Media]/Download/MediaDownloader.cs:247: // content range is null in case the server doesn't adhere the media download protocol, in
On 2013/10/04 20:12:23, ngmiceli wrote:
> "in the case" -> "when"
Done.
https://codereview.appspot.com/13480044/diff/12001/Src/GoogleApis/Apis/[Media]/Download/MediaDownloader.cs#newcode249
Src/GoogleApis/Apis/[Media]/Download/MediaDownloader.cs:249: currentRequestFirstBytePos = mediaContentLength =
I ran it with excel sheet in the drive API.
I didn't add test for it yet. All the trick is to stop after the first request, because range header is empty, so this is the first and last chunk. nothing special
On 2013/10/04 20:12:23, ngmiceli wrote:
> Is this logic tested?
Message from ngmiceli@google.com
2013-10-14T19:27:10+00:00ngmiceliurn:md5:8865ff1dace7b5fbe609a9c815e88851
LGTM
Just patch up documentation
https://codereview.appspot.com/13480044/diff/22001/Src/GoogleApis/Apis/Http/ConfigurableMessageHandler.cs
File Src/GoogleApis/Apis/Http/ConfigurableMessageHandler.cs (right):
https://codereview.appspot.com/13480044/diff/22001/Src/GoogleApis/Apis/Http/ConfigurableMessageHandler.cs#newcode98
Src/GoogleApis/Apis/Http/ConfigurableMessageHandler.cs:98: /// <see cref="NumTries"/> to 1 and <see cref="NumRedirect"/> to 5, a retry will happen only for 5 redirects.
Try to be as explicit as possible. Here's what we came up with together:
"to 5, the library will send up to five redirect requests, but will not send any retry requests due to an error HTTP status code."