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

ExecuteActionMetadata requires digest_function #311

Merged
merged 1 commit into from
Sep 24, 2024

Conversation

werkt
Copy link
Collaborator

@werkt werkt commented Sep 24, 2024

The specified ExecuteActionMetadata will have no association with the digest_function requested in an execution, and cannot be used to retrieve the action, without a digest_function associated with it.

@werkt werkt requested a review from bergsieker as a code owner September 24, 2024 02:24
The specified ExecuteActionMetadata will have no association with the
digest_function requested in an execution, and cannot be used to
retrieve the action, without a digest_function associated with it.
@werkt werkt force-pushed the eam-digest-function branch from c26335b to 00598d0 Compare September 24, 2024 02:33
@bergsieker bergsieker merged commit a6328f5 into bazelbuild:main Sep 24, 2024
1 check passed
@werkt werkt deleted the eam-digest-function branch September 24, 2024 13:19
EdSchouten added a commit to buildbarn/bb-remote-execution that referenced this pull request Sep 24, 2024
This field just got added to the protocol in the following PR:

bazelbuild/remote-apis#311

For us it's trivial to expose this information, as we already have it
stored in the Remote Worker desired state message. There it is
guaranteed to be set, even for the traditional digest functions that
don't require it.
EdSchouten added a commit to EdSchouten/remote-apis that referenced this pull request Sep 24, 2024
- Unlike the other messages where a digest_function field was added,
  ExecuteActionMetadata is returned by the server to the client. This
  means that the words "client" and "server" need to be swapped around
  in some but not all places.

- To ensure backward compatibility, we permit digest functions that were
  defined at the time this field was added to remain unset. When the
  digest_function fields were added to the other messages, the last one
  to be added was MURMUR3. In the meantime we've added BLAKE3 and
  SHA256TREE, so for this specific field we must list those as well.
EdSchouten added a commit to EdSchouten/remote-apis that referenced this pull request Sep 24, 2024
- Unlike the other messages where a digest_function field was added,
  ExecuteActionMetadata is returned by the server to the client. This
  means that the words "client" and "server" need to be swapped around
  in some but not all places.

- To ensure backward compatibility, we permit digest functions that were
  defined at the time this field was added to remain unset. When the
  digest_function fields were added to the other messages, the last one
  to be added was MURMUR3. In the meantime we've added BLAKE3 and
  SHA256TREE, so for this specific field we must list those as well.
EdSchouten added a commit to buildbarn/bb-remote-execution that referenced this pull request Sep 24, 2024
This field just got added to the protocol in the following PR:

bazelbuild/remote-apis#311

For us it's trivial to expose this information, as we already have it
stored in the Remote Worker desired state message. There it is
guaranteed to be set, even for the traditional digest functions that
don't require it.
EdSchouten added a commit that referenced this pull request Sep 26, 2024
- Unlike the other messages where a digest_function field was added,
  ExecuteActionMetadata is returned by the server to the client. This
  means that the words "client" and "server" need to be swapped around
  in some but not all places.

- To ensure backward compatibility, we permit digest functions that were
  defined at the time this field was added to remain unset. When the
  digest_function fields were added to the other messages, the last one
  to be added was MURMUR3. In the meantime we've added BLAKE3 and
  SHA256TREE, so for this specific field we must list those as well.
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