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

Fix:Prevent CAS and AC from using the same store and add integration … #1177

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

Conversation

RickDeb2004
Copy link

@RickDeb2004 RickDeb2004 commented Jul 19, 2024

…test

Description

This commit includes changes to prevent CAS and AC from being configured to use the same store, which could cause conflicts. The conflict check logic has been optimized using a hashmap for better performance. Additionally, an integration test script has been added to verify the conflict detection functionality.

Fixes # (issue)
issue: #768

Type of change

Please delete options that aren't relevant.

  • Bug fix (non-breaking change which fixes an issue)

  • Breaking change (fix or feature that would cause existing functionality to
    not work as expected)

  • This change requires a documentation update

How Has This Been Tested?

Verified the service logs for correct error messages when CAS and AC are configured with the same store.
Created and executed an integration test script simple_store_conflict_test.sh to automate the verification process.

Checklist

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

This change is Reviewable

@CLAassistant
Copy link

CLAassistant commented Jul 19, 2024

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@aleksdmladenovic aleksdmladenovic left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 LGTMs obtained, and pending CI: Bazel Dev / ubuntu-22.04, Cargo Dev / macos-13, Cargo Dev / ubuntu-22.04, Installation / macos-13, Installation / macos-14, Installation / ubuntu-22.04, Publish image, Publish nativelink-worker-init, Publish nativelink-worker-lre-cc, Remote / large-ubuntu-22.04, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), integration-tests (20.04), integration-tests (22.04), macos-13, pre-commit-checks, ubuntu-20.04 / stable, ubuntu-22.04, ubuntu-22.04 / stable, windows-2022 / stable, and 1 discussions need to be resolved


src/bin/nativelink.rs line 251 at r1 (raw file):

            }
            Ok(())
        }

cas_config and ac_conf are already HashMaps.
They are not Array.

cc: @RickDeb2004

@MarcusSorealheis
Copy link
Collaborator

@RickDeb2004 You need to sign the commit and fix the tests failing. I can help but ideally you should handle this one.

@MarcusSorealheis
Copy link
Collaborator

Let me know in Slack if you want a hand.

@RickDeb2004
Copy link
Author

@RickDeb2004 You need to sign the commit and fix the tests failing. I can help but ideally you should handle this one.

Sure sir , I will try to fix everything by tonight

@allada
Copy link
Member

allada commented Aug 16, 2024

Any update on this? If not I want to put this ticket back into the queue.

Thanks :-)

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.

5 participants