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

Set the digest_function field as part of all relevant gRPC requests #17772

Conversation

EdSchouten
Copy link
Contributor

@EdSchouten EdSchouten commented Mar 14, 2023

In the following PR we extended most of the REv2 *Request messages to take an explicit digest function:

bazelbuild/remote-apis#235

This eliminates the ambiguity between digest functions that have the same hash length (e.g., MD5 and MURMUR3, SHA256 and SHA256TREE). This change extends the REv2 client in Bazel to set the digest_function field explicitly, so that the server does not need to use any heuristics to pick a digest function when processing requests.

As we are now going to call DigestUtil.getDigestFunction() far more
frequently, alter DigestUtil to precompute the value upon construction.

This change was tested by letting Bazel forward all traffic through an instance of bb_clientd that was patched up to require the use of digest_function.

@EdSchouten EdSchouten force-pushed the eschouten/20230314-set-digest-function branch 4 times, most recently from e693925 to 1a5e926 Compare March 14, 2023 19:56
@EdSchouten EdSchouten marked this pull request as ready for review March 14, 2023 20:09
@EdSchouten EdSchouten requested a review from a team as a code owner March 14, 2023 20:09
@EdSchouten EdSchouten requested review from coeuvre and sgowroji March 14, 2023 20:09
@EdSchouten
Copy link
Contributor Author

Hey @coeuvre @sgowroji,
This PR brings in the latest version of REv2 like #16791 does. Is this not a problem or should I wait until #16791 is merged and rebase?

@ShreeM01 ShreeM01 added team-Remote-Exec Issues and PRs for the Execution (Remote) team awaiting-review PR is awaiting review from an assigned reviewer labels Mar 14, 2023
@coeuvre
Copy link
Member

coeuvre commented Mar 15, 2023

Please wait until #16791 is merged and rebase. Thanks!

In the following PR we extended most of the REv2 *Request messages to
take an explicit digest function:

bazelbuild/remote-apis#235

This eliminates the ambiguity between digest functions that have the
same hash length (e.g., MD5 and MURMUR3, SHA256 and SHA256TREE). This
change extends the REv2 client in Bazel to set the digest_function field
explicitly, so that the server does not need to use any heuristics to
pick a digest function when processing requests.

As we are now going to call DigestUtil.getDigestFunction() far more
frequently, alter DigestUtil to precompute the value upon construction.
@EdSchouten EdSchouten force-pushed the eschouten/20230314-set-digest-function branch from 1a5e926 to 96e6596 Compare March 16, 2023 07:03
@EdSchouten
Copy link
Contributor Author

Done!

@sgowroji
Copy link
Member

Hi @coeuvre, Since I can see that this PR has been approved, please let me know whether I should proceed with importing it.Thanks!

@EdSchouten
Copy link
Contributor Author

Friendly ping to @coeuvre, as he might have missed @sgowroji's question.

@tjgq
Copy link
Contributor

tjgq commented Mar 23, 2023

I had a look and it SGTM. Let's import it.

@tjgq tjgq added awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally and removed awaiting-review PR is awaiting review from an assigned reviewer labels Mar 23, 2023
@ShreeM01 ShreeM01 removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Mar 28, 2023
@brentleyjones
Copy link
Contributor

Can/should this be cherry-picked into 6.2?

@EdSchouten
Copy link
Contributor Author

If it is cherry-picked into 6.2, maybe #16791 should be as well?

fweikert pushed a commit to fweikert/bazel that referenced this pull request May 25, 2023
In the following PR we extended most of the REv2 *Request messages to take an explicit digest function:

bazelbuild/remote-apis#235

This eliminates the ambiguity between digest functions that have the same hash length (e.g., MD5 and MURMUR3, SHA256 and SHA256TREE). This change extends the REv2 client in Bazel to set the digest_function field explicitly, so that the server does not need to use any heuristics to pick a digest function when processing requests.

As we are now going to call DigestUtil.getDigestFunction() far more
frequently, alter DigestUtil to precompute the value upon construction.

This change was tested by letting Bazel forward all traffic through an instance of bb_clientd that was patched up to require the use of `digest_function`.

Closes bazelbuild#17772.

PiperOrigin-RevId: 520074622
Change-Id: Ie92dc368c502b9bc3fddef56a5eb0a238760f673
brentleyjones pushed a commit to brentleyjones/bazel that referenced this pull request Jul 25, 2023
In the following PR we extended most of the REv2 *Request messages to take an explicit digest function:

bazelbuild/remote-apis#235

This eliminates the ambiguity between digest functions that have the same hash length (e.g., MD5 and MURMUR3, SHA256 and SHA256TREE). This change extends the REv2 client in Bazel to set the digest_function field explicitly, so that the server does not need to use any heuristics to pick a digest function when processing requests.

As we are now going to call DigestUtil.getDigestFunction() far more
frequently, alter DigestUtil to precompute the value upon construction.

This change was tested by letting Bazel forward all traffic through an instance of bb_clientd that was patched up to require the use of `digest_function`.

Closes bazelbuild#17772.

PiperOrigin-RevId: 520074622
Change-Id: Ie92dc368c502b9bc3fddef56a5eb0a238760f673
(cherry picked from commit 0a8380b)
brentleyjones pushed a commit that referenced this pull request Jul 25, 2023
In the following PR we extended most of the REv2 *Request messages to take an explicit digest function:

bazelbuild/remote-apis#235

This eliminates the ambiguity between digest functions that have the same hash length (e.g., MD5 and MURMUR3, SHA256 and SHA256TREE). This change extends the REv2 client in Bazel to set the digest_function field explicitly, so that the server does not need to use any heuristics to pick a digest function when processing requests.

As we are now going to call DigestUtil.getDigestFunction() far more
frequently, alter DigestUtil to precompute the value upon construction.

This change was tested by letting Bazel forward all traffic through an instance of bb_clientd that was patched up to require the use of `digest_function`.

Closes #17772.

PiperOrigin-RevId: 520074622
Change-Id: Ie92dc368c502b9bc3fddef56a5eb0a238760f673
(cherry picked from commit 0a8380b)
@iancha1992
Copy link
Member

The changes in this PR have been included in Bazel 6.4.0 RC1. Please test out the release candidate and report any issues as soon as possible. If you're using Bazelisk, you can point to the latest RC by setting USE_BAZEL_VERSION=last_rc.
Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-Remote-Exec Issues and PRs for the Execution (Remote) team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants