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

Support digest function in GrpcStore #1548

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

asr2003
Copy link

@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
Collaborator

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

@asr2003
Copy link
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
Member

@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
Author

asr2003 commented Dec 18, 2024

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

@asr2003
Copy link
Author

asr2003 commented Dec 18, 2024

@aaronmondal To validate the is_digest_function , I have added the tests and grpc_ store unit tests for read and has_with_results I have mocked store and I am not sure there in right way but the changes added here of read will refelected in the https://github.com/TraceMachina/nativelink/blob/main/integration_tests/simple_remote_execution_test.sh
which I guess GRPCStore runs underhood and I have also faced before some test fail from it of same read fail when I have worked on it and which I used for evaluation of this PR changes.

Here's what I have mocked for and looking part of unit test but fails :)

    async fn test_read_with_valid_digest_function() {
        let config = Retry::default();

        let mock_grpc_store = GrpcStore {
            instance_name: "test".to_string(),
            store_type: nativelink_config::stores::StoreType::cas,
            retrier: Retrier::new(
                Arc::new(|_| Box::pin(sleep(Duration::from_secs(0)))),
                Arc::new(|delay: Duration| delay),
                config,
            ),
            connection_manager: ConnectionManager::new(
                vec!["test-endpoint"]
                    .into_iter()
                    .map(|endpoint_str| {
                        let grpc_endpoint = GrpcEndpoint {
                            address: endpoint_str.to_string(),
                            tls_config: None,                
                            concurrency_limit: Some(1),       
                        };
                        tls_utils::endpoint(&grpc_endpoint).unwrap()
                    })
                    .collect::<Vec<_>>(),
                1,
                1,
                Retry::default(),
                Arc::new(|_| Duration::from_secs(0)),
            ),
        };

        let read_request = ReadRequest {
            resource_name: "resource-sha256".to_string(),
            read_offset: 0,
            read_limit: 10,
        };

        let result = mock_grpc_store_.read(read_request).await;
        assert!(result.is_ok(), "Read should succeed with supported digest function");
    }

Correct me if I am wrong in mocking GRPC Store for unit tests

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
4 participants