nativelink icon indicating copy to clipboard operation
nativelink copied to clipboard

NativeLink Should Not Start if CAS and AC are the Same

Open MarcusSorealheis opened this issue 1 year ago • 4 comments

If users try to start NativeLink configuring CAS and AC to use the same store, it should fail and provide a helpful error.

MarcusSorealheis avatar Mar 16 '24 18:03 MarcusSorealheis

It is related to: https://github.com/TraceMachina/nativelink/issues/744

MarcusSorealheis avatar Mar 16 '24 18:03 MarcusSorealheis

I'd like to work on this issue. cc: @allada , @MarcusSorealheis

boldpulse avatar Jul 17 '24 12:07 boldpulse

I resolved the issue , but as @aleksdmladenovic raised a pr already , I am not raising my pr now without your permission @MarcusSorealheis . But while reviewing his PR , I just want to suggest the modifications needed:

  1. to reduce time complexity I created a HashMap called cas_store_map from the CAS configuration, where the key is the instance_name and the value is a reference to the cas_store.

  2. then iterate over the AC configuration and check if the instance_name exists in the cas_store_map.

  3. we compare the store names, and if they match, we return an error.

I have added a integration test file also in my solution simple_store_conflict_test.sh in the integration_tests directory

Want to know what I have written in my script?

1)A function check_store_conflict to simulate checking for conflicts between CAS and AC configurations.

2)Two test cases: test_conflict_when_cas_and_ac_use_same_store to verify that a conflict is detected when CAS and AC use the same store. test_no_conflict_when_cas_and_ac_use_different_stores to verify that no conflict is detected when CAS and AC use different stores. The script runs both test cases and prints the result of each test.

@MarcusSorealheis need your permission to raise my PR , otherwise I can assist @aleksdmladenovic in his PR as being an active contributor , I always respect opensource rules

RickDeb2004 avatar Jul 19 '24 18:07 RickDeb2004

Raise a PR

Marcus Eagan

On Fri, Jul 19, 2024 at 11:33 DEBANJAN MUKHERJEE @.***> wrote:

I resolved the issue , but as @aleksdmladenovic https://github.com/aleksdmladenovic raised a pr already , I am not raising my pr now without your permission @MarcusSorealheis https://github.com/MarcusSorealheis . But while reviewing his PR , I just want to suggest the modifications needed:

to reduce time complexity I created a HashMap called cas_store_map from the CAS configuration, where the key is the instance_name and the value is a reference to the cas_store. 2.

then iterate over the AC configuration and check if the instance_name exists in the cas_store_map. 3.

we compare the store names, and if they match, we return an error.

I have added a integration test file also in my solution simple_store_conflict_test.sh in the integration_tests directory

Want to know what I have written in my script?

1)A function check_store_conflict to simulate checking for conflicts between CAS and AC configurations.

2)Two test cases: test_conflict_when_cas_and_ac_use_same_store to verify that a conflict is detected when CAS and AC use the same store. test_no_conflict_when_cas_and_ac_use_different_stores to verify that no conflict is detected when CAS and AC use different stores. The script runs both test cases and prints the result of each test.

@MarcusSorealheis https://github.com/MarcusSorealheis need your permission to raise my PR , otherwise I can assist @aleksdmladenovic https://github.com/aleksdmladenovic in his PR as being an active contributor , I always respect opensource rules

— Reply to this email directly, view it on GitHub https://github.com/TraceMachina/nativelink/issues/768#issuecomment-2239882290, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAR6TSHAYJYK2IRY7BS4N5TZNFLXVAVCNFSM6AAAAABEZQZK7OVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMZZHA4DEMRZGA . You are receiving this because you were mentioned.Message ID: @.***>

MarcusSorealheis avatar Jul 19 '24 19:07 MarcusSorealheis