feat(gax): add ResumableUploadCallable and ResumableUploadCallSettings - #14052
feat(gax): add ResumableUploadCallable and ResumableUploadCallSettings#14052blakeli0 wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces the ResumableUploadCallSettings and ResumableUploadCallable classes to support transport-independent resumable uploads, along with associated unit tests. The review feedback suggests adding input validation to ensure chunkSize is positive and totalBytes is non-negative, as well as removing several redundant null casts in the overloaded call and futureCall methods to clean up the code.
| public Builder<RequestT, ResponseT> setChunkSize(Integer chunkSize) { | ||
| this.chunkSize = chunkSize; | ||
| return this; | ||
| } |
There was a problem hiding this comment.
It is highly recommended to validate that chunkSize is positive when it is configured. This prevents invalid configurations (such as negative or zero chunk sizes) from being set and causing runtime failures during uploads.
public Builder<RequestT, ResponseT> setChunkSize(Integer chunkSize) {
if (chunkSize != null) {
com.google.common.base.Preconditions.checkArgument(
chunkSize > 0, "chunkSize must be positive: %s", chunkSize);
}
this.chunkSize = chunkSize;
return this;
}| public Builder<RequestT, ResponseT> setTotalBytes(Long totalBytes) { | ||
| this.totalBytes = totalBytes; | ||
| return this; | ||
| } |
There was a problem hiding this comment.
It is highly recommended to validate that totalBytes is non-negative when configured. This prevents invalid negative values from being set.
public Builder<RequestT, ResponseT> setTotalBytes(Long totalBytes) {
if (totalBytes != null) {
com.google.common.base.Preconditions.checkArgument(
totalBytes >= 0, "totalBytes must be non-negative: %s", totalBytes);
}
this.totalBytes = totalBytes;
return this;
}| public ApiFuture<ResponseT> futureCall( | ||
| RequestT request, | ||
| InputStream payload, | ||
| ResumableUploadCallSettings<RequestT, ResponseT> perRequestSettings) { | ||
| return futureCall(request, payload, perRequestSettings, (ApiCallContext) null); | ||
| } |
There was a problem hiding this comment.
The cast (ApiCallContext) null is redundant because there is only one 4-argument overload of futureCall, so the compiler can resolve the method call unambiguously. Removing the redundant cast improves code readability.
| public ApiFuture<ResponseT> futureCall( | |
| RequestT request, | |
| InputStream payload, | |
| ResumableUploadCallSettings<RequestT, ResponseT> perRequestSettings) { | |
| return futureCall(request, payload, perRequestSettings, (ApiCallContext) null); | |
| } | |
| public ApiFuture<ResponseT> futureCall( | |
| RequestT request, | |
| InputStream payload, | |
| ResumableUploadCallSettings<RequestT, ResponseT> perRequestSettings) { | |
| return futureCall(request, payload, perRequestSettings, null); | |
| } |
| public ApiFuture<ResponseT> futureCall(RequestT request, InputStream payload) { | ||
| return futureCall( | ||
| request, payload, (ResumableUploadCallSettings<RequestT, ResponseT>) null, (ApiCallContext) null); | ||
| } |
There was a problem hiding this comment.
The casts (ResumableUploadCallSettings<RequestT, ResponseT>) null and (ApiCallContext) null are redundant because there is only one 4-argument overload of futureCall. Removing them simplifies the code and improves readability.
public ApiFuture<ResponseT> futureCall(RequestT request, InputStream payload) {
return futureCall(request, payload, null, null);
}| public ResponseT call( | ||
| RequestT request, | ||
| InputStream payload, | ||
| ResumableUploadCallSettings<RequestT, ResponseT> perRequestSettings) { | ||
| return call(request, payload, perRequestSettings, (ApiCallContext) null); | ||
| } |
There was a problem hiding this comment.
The cast (ApiCallContext) null is redundant because there is only one 4-argument overload of call. Removing it improves readability.
| public ResponseT call( | |
| RequestT request, | |
| InputStream payload, | |
| ResumableUploadCallSettings<RequestT, ResponseT> perRequestSettings) { | |
| return call(request, payload, perRequestSettings, (ApiCallContext) null); | |
| } | |
| public ResponseT call( | |
| RequestT request, | |
| InputStream payload, | |
| ResumableUploadCallSettings<RequestT, ResponseT> perRequestSettings) { | |
| return call(request, payload, perRequestSettings, null); | |
| } |
| public ResponseT call(RequestT request, InputStream payload) { | ||
| return call( | ||
| request, payload, (ResumableUploadCallSettings<RequestT, ResponseT>) null, (ApiCallContext) null); | ||
| } |
There was a problem hiding this comment.
The casts (ResumableUploadCallSettings<RequestT, ResponseT>) null and (ApiCallContext) null are redundant because there is only one 4-argument overload of call. Removing them simplifies the code.
public ResponseT call(RequestT request, InputStream payload) {
return call(request, payload, null, null);
}483ea79 to
c52d225
Compare
Clean up ResumableUploadCallSettingsTest JUnit 5 test suite by removing redundant equals/hashCode test for @autovalue. Add ResumableUploadCallable abstract base class.
c52d225 to
2cf5521
Compare
Description
This PR introduces the foundational GAX Public API surface for HTTP/JSON resumable uploads.
Changes
@AutoValueconfiguration class withmerge(other)method andchunkSize = 8MBdefault.futureCall(request, payload, settings)method.merge(other)overrides.