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

don't require explicit empty blob uploads for HTTP either #234

Merged
merged 1 commit into from
Mar 24, 2020

Conversation

mostynb
Copy link
Collaborator

@mostynb mostynb commented Mar 23, 2020

The gRPC API implementation has a hack to handle clients (like bazel?) which in some cases don't upload the empty blob, but do reference it. It is kind of OK not to upload the empty blob, since it can be recognised by hash and trivially regenerated.

Let's imlpement the hack for the HTTP API also.

Implements #233.

@mostynb
Copy link
Collaborator Author

mostynb commented Mar 23, 2020

@brentleyjones: would you like to cherry-pick + test this with your setup?

@brentleyjones
Copy link

@mostynb Yes! I'll report back soon-ish. Thanks!

@brentleyjones
Copy link

@mostynb This patch worked. BuildBarn is now able to validate AC's for HTTP mode with this patch.

Sadly infrastructure issues are preventing me from continuing to use HTTP mode, so I'll also have to followup on #231 testing for you.

@brentleyjones
Copy link

brentleyjones commented Mar 23, 2020

gRPC-instance1.txt
gRPC-instance2.txt
HTTP-instance1.txt
HTTP-instance2.txt

I inserted a newline between the two builds. As you can see in instance 2 for gRPC, it can't find ed11bb857931deee5257fae1ec0aeb51fc2e8be493c4af44315108c485613543, but in instance 2 for HTTP it can.

I started these with --disable_http_ac_validation true --disable_grpc_ac_deps true.

@mostynb
Copy link
Collaborator Author

mostynb commented Mar 23, 2020

Are the logs above from a test branch with both this change and the commit from #231? If so, then the logic for disabling the gRPC AC dependency check must be broken- I can't see any GRPC NODEPS AC GET ... OK log lines.

@brentleyjones
Copy link

Yes, it has both commits from this and #231. This PR worked perfectly and can be merged, IMO. (I thought I was commenting in #231.)

The gRPC API implementation has a hack to handle clients (like bazel?)
which in some cases don't upload the empty blob, but do reference it.
It is kind of OK not to upload the empty blob, since it can be recognised
by hash and trivially regenerated.

Let's imlpement the hack for the HTTP API also.

Implements buchgr#233.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants