feat(gax): implement startUploadCallable for resumable uploads - #14139
feat(gax): implement startUploadCallable for resumable uploads#14139whowes wants to merge 1 commit into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
whowes
changed the title
8d52acf to
7df9201
Compare
|
/gemini review |
7df9201 to
4253855
Compare
There was a problem hiding this comment.
Code Review
This pull request introduces HttpJsonResumableUploadClient to support resumable uploads over HTTP/JSON, along with a comprehensive suite of unit tests. The feedback suggests validating that the parsed chunk granularity is strictly positive to prevent potential arithmetic errors or infinite loops.
4253855 to
798876d
Compare
whowes
changed the title
798876d to
a071113
Compare
a071113 to
e43a3fa
Compare
3039c34 to
56d1462
Compare
56d1462 to
22569f9
Compare
7072e2b to
0b792ed
Compare
0b792ed to
5b6cbd7
Compare
5b6cbd7 to
a139b72
Compare
826210c to
8ebae80
Compare
8ebae80 to
295f162
Compare
2e01dcb to
d41b2ef
Compare
d41b2ef to
64defcf
Compare
64defcf to
04f09c2
Compare
04f09c2 to
7602831
Compare
7602831 to
9eab629
Compare
| .setFullMethodName(methodDescriptor.getFullMethodName()) | ||
| .setHttpMethod(HttpMethods.POST) | ||
| .setType(ApiMethodDescriptor.MethodType.UNARY) | ||
| .setRequestFormatter(methodDescriptor.getRequestFormatter()) |
There was a problem hiding this comment.
This is the requestFormatter generated in the stub so we can use it to parse a proto message to Json request?
There was a problem hiding this comment.
Correct, this will come from the stub.
| } | ||
|
|
||
| @Override | ||
| public void onHeaders(HttpJsonMetadata responseHeaders) { |
There was a problem hiding this comment.
IIUC, we need a custom listener because it is the only place we can get info from response headers?
There was a problem hiding this comment.
Right - this is directly related to #14136
| if (!Strings.isNullOrEmpty(granularityStr)) { | ||
| try { | ||
| this.chunkGranularity = Long.parseLong(granularityStr); | ||
| } catch (NumberFormatException ignored) { |
There was a problem hiding this comment.
I'm not sure we want to ignore it because it may indicate a bigger problem in the server. We may want to recreate an ApiException.
| /* retryable= */ false); | ||
| future.setException(apiException); | ||
| } | ||
| } catch (Throwable t) { |
There was a problem hiding this comment.
If there are any unknown exceptions happen in onClose(), I think it's OK to let it bubble up without recreating an ApiException.
There was a problem hiding this comment.
Done, switched to just setting it on the Future directly.
| .build()); | ||
| } else { | ||
| future.setException( | ||
| ApiExceptionFactory.createException( |
There was a problem hiding this comment.
If we are going to use HttpJsonCallableFactory (which provides other features like retry and tracing) to wrap this callable later, we don't need to recreate an ApiException because it is already supported by HttpJsonExceptionCallable.
There was a problem hiding this comment.
I added a helper method to wrap the startUpload callable in the HttpJsonExceptionCallable (and others in next PRs) so that we don't have to translate exceptions manually. We do still need to create ApiExceptions for occasions where a response code or missing/malformed header value signifies an error condition (where there's not an exception present already).
We should discuss more about Callable wrapping with later PRs - I'm not sure that the callables returned by this client should be wrapped for tracing and retries? Definitely I think the higher-level ResumableUploadCallables should be (in HttpJsonCallableFactory), but with these calls being managed by the state machine layer it's less clear to me how that will work (and I was having some inconsistent results in my local testing when wrapping in the tracing callable in particular.)
9eab629 to
7ec0238
Compare
7ec0238 to
33af57f
Compare
33af57f to
0f24dae
Compare
…es (googleapis#14138) HTTP/JSON client implementation for startUpload is in googleapis#14139.
0f24dae to
1dc0716
Compare
|
|
2 participants
The implementation uses a custom
HttpJsonClientCall.Listenerto extractX-Goog-Upload-URLandX-Goog-Upload-Chunk-Granularityheaders to be returned in aResumableUploadSession.