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.

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

ByteStreamUploader: accept ALREADY_EXISTS #12112

Closed
wants to merge 1 commit into from

Conversation

ulfjack
Copy link
Contributor

@ulfjack ulfjack commented Sep 16, 2020

If the service returns an ALREADY_EXISTS error, then we assume that the
proper file is present remotely.

Fixes #12111.

Change-Id: I7f5af0cca2e5efea32067b092f619c6593af0aac

If the service returns an ALREADY_EXISTS error, then we assume that the
proper file is present remotely.

Fixes bazelbuild#12111.

Change-Id: I7f5af0cca2e5efea32067b092f619c6593af0aac
@ulfjack
Copy link
Contributor Author

ulfjack commented Sep 16, 2020

@EricBurnett to double-check my reasoning here.

@EricBurnett
Copy link

SGTM on the change to ignore ALREADY_EXISTS - no harm in it.

Unconvinced on servers returning it though - do we need to document anything more explicitly in the REAPI? I think semantically ALREADY_EXISTS means what it says on the tin, but I suspect most clients of the REAPI ByteStream will not special-case it today, as you're seeing here. So while I think it's completely reasonable for clients to tolerate it, I wouldn't recommend any servers return it today, not without something stronger in the API text ("SHOULD treat ALREADY_EXISTS the same as a successful upload..."). Which would take some discussion in the community.

You can get around all this by returning OK instead of ALREADY_EXISTS, including without having first read all the incoming bytes - gRPC allows this (see grpc/grpc#7032).

And so that's my recommendation in general - anywhere you're looking to return ALREADY_EXISTS you'd probably be better off returning a preemptive 'OK' instead. Most clients (including this SDK) should work out of the box with that, and I'm not aware of any semantic reason to need to return ALREADY_EXISTS (clients don't actually care, they just want to know the file is uploaded).

@ulfjack
Copy link
Contributor Author

ulfjack commented Sep 16, 2020

We are doing that right now. The reason I was looking at the code is that we have tested one closed-source client which seems to misbehave when we prematurely return OK, although we're not sure why.

@philwo philwo requested a review from coeuvre September 23, 2020 15:15
@philwo philwo added the team-Remote-Exec Issues and PRs for the Execution (Remote) team label Sep 23, 2020
@coeuvre
Copy link
Member

coeuvre commented Sep 25, 2020

@ulfjack Can you rebase and fix the tests?

@ulfjack ulfjack requested a review from a team as a code owner December 2, 2021 17:58
@ckolli5 ckolli5 added the awaiting-review PR is awaiting review from an assigned reviewer label Apr 26, 2022
@coeuvre coeuvre added awaiting-user-response Awaiting a response from the author and removed awaiting-review PR is awaiting review from an assigned reviewer labels Apr 27, 2022
@keertk
Copy link
Member

keertk commented Dec 7, 2022

Hi there! Thank you for contributing to the Bazel repository. We appreciate your time and effort. We're doing a clean up of old PRs and will be closing this one since it seems to have stalled. Please feel free to reopen if you’re still interested in pursuing this or if you'd like to discuss anything further. We’ll respond as soon as we have the bandwidth/resources to do so.

@keertk keertk closed this Dec 7, 2022
Yannic added a commit to EngFlow/bazel that referenced this pull request Mar 8, 2023
If the service returns an `ALREADY_EXISTS` error, then we assume that
the proper file is present remotely.

Prior art: bazelbuild#12112

Fixes bazelbuild#12111
copybara-service bot pushed a commit that referenced this pull request Mar 9, 2023
If the service returns an `ALREADY_EXISTS` error, then we assume that the proper file is present remotely.

Prior art: #12112

Fixes #12111

Closes #17692.

PiperOrigin-RevId: 515339566
Change-Id: Iafdd288148e47197cc047d39c9a5e5b6c95acee1
ShreeM01 pushed a commit to ShreeM01/bazel that referenced this pull request Mar 10, 2023
If the service returns an `ALREADY_EXISTS` error, then we assume that the proper file is present remotely.

Prior art: bazelbuild#12112

Fixes bazelbuild#12111

Closes bazelbuild#17692.

PiperOrigin-RevId: 515339566
Change-Id: Iafdd288148e47197cc047d39c9a5e5b6c95acee1
ShreeM01 added a commit that referenced this pull request Mar 13, 2023
If the service returns an `ALREADY_EXISTS` error, then we assume that the proper file is present remotely.

Prior art: #12112

Fixes #12111

Closes #17692.

PiperOrigin-RevId: 515339566
Change-Id: Iafdd288148e47197cc047d39c9a5e5b6c95acee1

Co-authored-by: Yannic Bonenberger <yannic@engflow.com>
fweikert pushed a commit to fweikert/bazel that referenced this pull request May 25, 2023
If the service returns an `ALREADY_EXISTS` error, then we assume that the proper file is present remotely.

Prior art: bazelbuild#12112

Fixes bazelbuild#12111

Closes bazelbuild#17692.

PiperOrigin-RevId: 515339566
Change-Id: Iafdd288148e47197cc047d39c9a5e5b6c95acee1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-user-response Awaiting a response from the author cla: yes team-Remote-Exec Issues and PRs for the Execution (Remote) team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bazel should accept ALREADY_EXISTS for byte stream uploads
7 participants