Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Split ResumableUpload class and introduced associated options #902

Merged
merged 2 commits into from
Jan 30, 2017
Merged

Split ResumableUpload class and introduced associated options #902

merged 2 commits into from
Jan 30, 2017

Conversation

evildour
Copy link
Contributor

This is a port and cleanup of some of the changes proposed in googleapis/google-cloud-dotnet#640, which will support resumable uploads using signed URLs.

Most of the changes involve a refactoring which pulls out most of ResumableUpload<TRequest> into a new base class: the non-generic ResumableUpload.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jan 19, 2017
@evildour
Copy link
Contributor Author

@chrisdunelm or @jskeet, this is ready for review (I cannot assign reviewers)

@jskeet
Copy link
Collaborator

jskeet commented Jan 19, 2017

Will take a look tomorrow morning (UK time)

jskeet
jskeet reviewed Jan 20, 2017
View reviewed changes
Copy link
Collaborator

@jskeet jskeet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm generally fine with this, with a couple of suggestions - Chris may have more thoughts. (Chris, let me know if you want more background.)

Src/Support/GoogleApis/Apis/[Media]/Upload/ResumableUpload.cs

/// <summary>
/// Gets or sets the length of the steam. Will be <see cref="UnknownSize" /> if the media content length is
/// unknown.
/// </summary>
private long StreamLength { get; set; }
internal long StreamLength { get; set; }

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

Src/Support/GoogleApis/Apis/[Media]/Upload/ResumableUpload.cs
@@ -412,7 +391,7 @@ public Task<IUploadProgress> UploadAsync()

try
{
UploadUri = await InitializeUpload(cancellationToken).ConfigureAwait(false);
UploadUri = await InitiateSessionAsync(cancellationToken).ConfigureAwait(false);

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

Src/Support/GoogleApis/Apis/[Media]/Upload/ResumableUpload.cs
/// </returns>
private async Task<Uri> InitializeUpload(CancellationToken cancellationToken)
protected virtual Task<Uri> InitiateSessionAsync(CancellationToken cancellationToken)

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

Src/Support/GoogleApis/Apis/[Media]/Upload/ResumableUpload.cs
/// The type of the body of this request. Generally this should be the metadata related to the content to be
/// uploaded. Must be serializable to/from JSON.
/// </typeparam>
public class ResumableUpload<TRequest> : ResumableUpload

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

Src/Support/GoogleApis/Apis/[Media]/Upload/ResumableUpload.cs
@@ -817,7 +915,7 @@ private HttpRequestMessage CreateInitializeRequest()

// init parameters
builder.AddParameter(RequestParameterType.Query, "key", Service.ApiKey);
builder.AddParameter(RequestParameterType.Query, "uploadType", "resumable");
builder.AddParameter(RequestParameterType.Query, UploadType, Resumable);

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

Src/Support/GoogleApis/Apis/[Media]/Upload/ResumableUploadOptions.cs
/// </remarks>
public string ServiceName { get; set; }

internal ConfigurableHttpClient ConfigurableHttpClient { get { return HttpClient as ConfigurableHttpClient; } }

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@evildour
Copy link
Contributor Author

PTAL. I added a test as well for the CreateFromUploadUri uploader.

jskeet
jskeet reviewed Jan 20, 2017
View reviewed changes
Copy link
Collaborator

@jskeet jskeet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't pretend I fully understand the test, in terms of chunkSizeDelta, but still looks fine to me. Chris, can you review?

Src/Support/GoogleApis/Apis/[Media]/Upload/ResumableUpload.cs
private Uri _initiatedUploadUri;

public InitiatedResumableUpload(Uri uploadUri, Stream contentStream, ResumableUploadOptions options)
:base(contentStream, options)

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@evildour
Copy link
Contributor Author

Actually, I'm not sure either. I just copied the previous test and modified it to pre-initiate a session and used that URI to create the "real" uploader. I assumed the rest of the test code made sense. Although, as I'm typing this, I realize that since we're not using the temp uploader to actually upload, the chunk size doesn't matter, so that's unnecessary. I'll remove it.

chrisdunelm
chrisdunelm approved these changes Jan 27, 2017
View reviewed changes
Copy link
Contributor

@chrisdunelm chrisdunelm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fairly sure this looks OK ;)
LGTM

Src/Support/GoogleApis/Apis/[Media]/MediaApiErrorHandling.cs
@@ -59,7 +72,7 @@ internal static class MediaApiErrorHandling
// as a cause, for example. The expectation is that the exception returned by this method (below)
// will be thrown by the caller.
}
return new GoogleApiException(service.Name, message)
return new GoogleApiException(name ?? string.Empty, message)

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@evildour
Copy link
Contributor Author

Thank you. Please merge when green (as I cannot).

@chrisdunelm chrisdunelm merged commit 012089c into googleapis:master Jan 30, 2017
@evildour evildour deleted the resumable-upload branch January 30, 2017 14:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jskeet jskeet jskeet left review comments

@chrisdunelm chrisdunelm chrisdunelm approved these changes

Assignees

@jskeet jskeet

@chrisdunelm chrisdunelm

Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@evildour @jskeet @chrisdunelm @googlebot