Skip to content

Support digest function in GrpcStore#1548

Open
asr2003 wants to merge 1 commit intoTraceMachina:mainfrom
asr2003:refactor/grpc-store
Open

Support digest function in GrpcStore#1548
asr2003 wants to merge 1 commit intoTraceMachina:mainfrom
asr2003:refactor/grpc-store

Conversation

@asr2003
Copy link
Copy Markdown

@asr2003 asr2003 commented Dec 17, 2024

Description

  • Updated ResourceInfo to default digest_function to sha256 if not provided.
  • Ensured backward compatibility for existing functionality.

Fixes #1325
/claim #1325

Type of change

Please delete options that aren't relevant.

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Please also list any relevant details for your test configuration
Tests added

Checklist

  • [] Updated documentation if needed
  • Tests added/amended
  • bazel test //... passes locally
  • PR is contained in a single commit, using git amend see some docs

This change is Reviewable

@MarcusSorealheis
Copy link
Copy Markdown
Collaborator

Please do not open up PRs that do not adhere to the checklist, unless they do not impact NativeLink.

@asr2003
Copy link
Copy Markdown
Author

asr2003 commented Dec 18, 2024

The failed check before isn't known what it is failed for seems a deployment failure! Hopefully the deployments succeeded now

Copy link
Copy Markdown
Contributor

@aaronmondal aaronmondal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+@allada @MarcusSorealheis What do we do about test coverage here? The grpcStore currently has no tests 😅 https://tracemachina.github.io/nativelink/coverage/build/source/nativelink-store/src/grpc_store.rs.html

@asr2003 I think it should be fine if we just cover the changed parts, i.e. unit tests for the read, has_with_results and is_supported_digest_function, and only tests that trigger the branches that you added (i.e. tests that force the new error handling and check that they're actually hit).

Reviewable status: 0 of 2 LGTMs obtained, and 0 of 2 files reviewed (waiting on @allada)

@asr2003
Copy link
Copy Markdown
Author

asr2003 commented Dec 18, 2024

Sure! I will add tests to ensure the branch coverage.

@asr2003
Copy link
Copy Markdown
Author

asr2003 commented Feb 8, 2025

@aaronmondal @MarcusSorealheis @allada Any update on this?

@laz-001
Copy link
Copy Markdown

laz-001 commented May 23, 2025

@aaronmondal @MarcusSorealheis @allada - is this expeted to go through? Otherwise may extend the bounty with the testing-extension?

@asr2003
Copy link
Copy Markdown
Author

asr2003 commented May 25, 2025

@aaronmondal @MarcusSorealheis I have added unit tests as per review comments for the logic added of digests in grpc_store. Can you have a review on this?

@asr2003
Copy link
Copy Markdown
Author

asr2003 commented Jun 5, 2025

@MarcusSorealheis @aaronmondal Would appreciate your review when you have time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

💎 Support digest_function in GrpcStore

5 participants