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

Add bazel flag for max_cas_entry size. #18449

Open
amishra-u opened this issue May 19, 2023 · 7 comments
Open

Add bazel flag for max_cas_entry size. #18449

amishra-u opened this issue May 19, 2023 · 7 comments
Assignees
Labels
help wanted Someone outside the Bazel team could own this P3 We're not considering working on this, but happy to review a PR. (No assignee) team-Performance Issues for Performance teams team-Remote-Exec Issues and PRs for the Execution (Remote) team type: feature request

Comments

@amishra-u
Copy link
Contributor

amishra-u commented May 19, 2023

Description of the feature request:

As of today, the max_cas_entry_size is declared at the server side. In the event that Bazel sends a write request containing a digest larger than the specified max_cas_entry_size, the remote cache throws an 'out_of_range' error. By introducing a Bazel flag for max_cas_entry_size, it becomes possible to validate it on the client side, potentially eliminating the need for a round trip to the server. This improvement has the potential to enhance build performance.

What underlying problem are you trying to solve with this feature?

Improved build performance with remote cache.

@sgowroji sgowroji added team-Performance Issues for Performance teams team-Remote-Exec Issues and PRs for the Execution (Remote) team labels May 19, 2023
@tjgq tjgq added P3 We're not considering working on this, but happy to review a PR. (No assignee) help wanted Someone outside the Bazel team could own this and removed untriaged labels May 30, 2023
@diegohavenstein
Copy link

@tjgq @sgowroji @Pavank1992 could you give an estimate of how much work this is? I'd be willing to help, but need to know roughly how much work it is. Sounds like passing through an argument and putting a conditional somewhere around the upload logic, but not sure if it is that simple

@tjgq
Copy link
Contributor

tjgq commented Oct 12, 2023

Before we decide to add a flag: is this something that could be negotiated with the server during the initial handshake? (We already negotiate other things, like the digest hash function used by the CAS.)

@amishra-u
Copy link
Contributor Author

@tjgq yes, remote-cache can send the max supported cas entry size on getcapablities call, this way we will not need to add any bazel flag (seems better option).
but we may need to add new property in the repsonse. https://github.com/bazelbuild/remote-apis/blob/main/build/bazel/remote/execution/v2/remote_execution.proto#L1968

@tjgq
Copy link
Contributor

tjgq commented Oct 12, 2023

In that case, could you bring this up as a potential addition to the spec by filing an issue at https://github.com/bazelbuild/remote-apis?

@diegohavenstein
Copy link

Sounds like a better approach indeed

@amishra-u can you please also update the feature request here and assign it to me? (I can't edit) Thx!

@amishra-u
Copy link
Contributor Author

@diegohavenstein same here, I don't have permission to assign the task to anyone.

@coeuvre
Copy link
Member

coeuvre commented Oct 16, 2023

@diegohavenstein I have assigned to you. Thanks!

diegohavenstein added a commit to diegohavenstein/remote-apis that referenced this issue Oct 16, 2023
We need this field to improve the performance of Bazel when using remote
cache, since by doing this we can determine the CAS entry upper limit size,
thus skipping uploads that would fail due to size constraints.

The issue on Bazel side is bazelbuild/bazel#18449
diegohavenstein added a commit to diegohavenstein/remote-apis that referenced this issue Oct 17, 2023
We need this field to improve the performance of Bazel when using remote
cache, since by doing this we can determine the CAS entry upper limit size,
thus skipping uploads that would fail due to size constraints.

The issue on Bazel side is bazelbuild/bazel#18449
diegohavenstein added a commit to diegohavenstein/remote-apis that referenced this issue Oct 18, 2023
We need this field to improve the performance of Bazel when using remote
cache, since by doing this we can determine the CAS blob upper limit size,
thus skipping uploads that would fail due to size constraints.

The issue on Bazel side is bazelbuild/bazel#18449
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Someone outside the Bazel team could own this P3 We're not considering working on this, but happy to review a PR. (No assignee) team-Performance Issues for Performance teams team-Remote-Exec Issues and PRs for the Execution (Remote) team type: feature request
Projects
None yet
Development

No branches or pull requests

6 participants