Skip to content

Conversation

@dinceraslancom
Copy link

@dinceraslancom dinceraslancom commented Apr 28, 2025

Description

This update fixes issues in RedisStore and RedisScheduler from #1585.

Use key_prefix to add a custom prefix to all keys.
Choose a db number instead of always using 0.

Fixes # (1585)

Type of change

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

How Has This Been Tested?

Configured nativelink-config/examples/redis.json5 with both key_prefix: "my_test_prefix:" and db: 1.
Started Redis locally using Docker.
Started Nativelink using bazel run //:nativelink -- $(pwd)/nativelink-config/examples/redis.json5
Used redis-cli MONITOR to watch Redis commands.
See Redis commands always using keys with my_test_prefix:.

Checklist

  • [ x ] bazel test //... passes locally
    This PR contains two distinct logical commits (one for key_prefix, one for db selection) for clarity in history

This change is Reviewable

@CLAassistant
Copy link

CLAassistant commented Apr 28, 2025

CLA assistant check
All committers have signed the CLA.

@MarcusSorealheis
Copy link
Collaborator

Thank you for the contribution. It's almost ready for review. Simple fix here:

 > index 38f06c8..5cd5b44 100644
       > --- a/nativelink-config/examples/redis.json5
       > +++ b/nativelink-config/examples/redis.json5
       > @@ -75,4 +75,4 @@
       >        }
       >      }
       >    ]
       > -}
       > \ No newline at end of file
       > +}

you only need an empty line at the end of /nativelink-config/examples/redis.json5.

Granted, I haven't tested the functionality, but I will. @barrbrain and @aaronmondal and @kubevalet will also be keen to test it.

@dinceraslancom
Copy link
Author

@MarcusSorealheis Thank you for the feedback. I've fixed that part.

@MarcusSorealheis
Copy link
Collaborator

I've merged with main to catch up and observe if anything new breaks.

@MarcusSorealheis
Copy link
Collaborator

@dinceraslancom You need to sign the CLA?

@MarcusSorealheis
Copy link
Collaborator

Could you also add a test?

@MarcusSorealheis MarcusSorealheis marked this pull request as draft April 30, 2025 19:01
@dinceraslancom
Copy link
Author

dinceraslancom commented May 1, 2025

@MarcusSorealheis I already confirmed the CLA, but there was an issue with email matching, solved.

I will write test case.

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.

3 participants