HttpCacheClient: make upload async #12115
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This matches the behavior for download, and improves performance for actions with a lot of large outputs. The code was already using futures in the API and in the implementation, but the glue code blocked on the internal future and returned an immediate future to the caller, rather than doing it async. Fixes bazelbuild#6091. Change-Id: I3151aa96b879323e0000d3209f6b9bc8be0066d4
|
Thanks for the PR! LGTM. But some tests failed, any idea with that? |
|
It was a bug in the change which I just fixed. |
|
All tests pass! |
coeuvre
approved these changes
Sep 18, 2020
meisterT
reviewed
Sep 18, 2020
|
|
||
| try { | ||
| Channel ch = channelAcquired.getNow(); | ||
| ChannelPipeline p = ch.pipeline(); |
There was a problem hiding this comment.
nit: can you please change p to pipeline (or similar)?
There was a problem hiding this comment.
Fixed. I changed the download side as well.
| SettableFuture<Void> result = SettableFuture.create(); | ||
| acquireUploadChannel() | ||
| .addListener( | ||
| (Future<Channel> chP) -> { |
There was a problem hiding this comment.
Changed. I didn't change the download side.
| uploadBlocking(digest.getHash(), digest.getSizeBytes(), in, /* casUpload= */ true); | ||
| } catch (IOException | InterruptedException e) { | ||
| try { | ||
| return uploadAsync(digest.getHash(), digest.getSizeBytes(), file.getInputStream(), true); |
There was a problem hiding this comment.
nit: please re-add /* casUpload= */
(same a few lines below and line 695)
There was a problem hiding this comment.
Sorry about that. Not sure how that got lost.
| if (cause instanceof HttpException) { | ||
| HttpResponse response = ((HttpException) cause).response(); | ||
| try { | ||
| // If the error is due to an expired auth token and we can reset the input stream, |
There was a problem hiding this comment.
Fixed. Also a couple more of these.
|
PTAL |
meisterT
approved these changes
Sep 21, 2020
Yannic
pushed a commit
to Yannic/bazel
that referenced
this pull request
Oct 5, 2020
This matches the behavior for download, and improves performance for actions with a lot of outputs. The code was already using futures in the API and in the implementation, but the glue code blocked on the internal future and returned an immediate future to the caller, rather than doing it async. Fixes bazelbuild#6091. Change-Id: I3151aa96b879323e0000d3209f6b9bc8be0066d4 Closes bazelbuild#12115. Change-Id: I72032f7b3e88de4d2e2b5abd2d89380f7fcc31e5 PiperOrigin-RevId: 332796965
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This matches the behavior for download, and improves performance for actions
with a lot of outputs.
The code was already using futures in the API and in the implementation,
but the glue code blocked on the internal future and returned an immediate
future to the caller, rather than doing it async.
Fixes #6091.
Change-Id: I3151aa96b879323e0000d3209f6b9bc8be0066d4