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

HLD for Redis Client Manager #1810

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

Conversation

ayelrod
Copy link

@ayelrod ayelrod commented Sep 20, 2024

This HLD describes a design for using Redis connection pools in sonic-mgmt-common to reduce stress on the Redis Unix/TCP socket.

Copy link

linux-foundation-easycla bot commented Sep 20, 2024

CLA Signed


The committers listed above are authorized under a signed CLA.


#### RedisClient(db DBNum)

This RedisClient function takes in a database number, defined in [db.go](https://github.com/sonic-net/sonic-mgmt-common/blob/master/translib/db/db.go#L136), and returns an existing Redis Client to the caller. This Redis Client can be shared across multiple goroutines and has a connection pool. It cannot be used for [Transactional Redis Operations](https://redis.io/docs/latest/develop/interact/transactions/) (such as MULTI, EXEC, PSUBSCRIBE, etc...).
Copy link
Collaborator

Choose a reason for hiding this comment

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

Today redis clients are created by db.NewDB(), which is called per translib API (Get, Create, Delete etc). Each translib API handling will involve multiple redis operations of different kinds. All of them are using the same redis client created during db.NewDB() call today. How do you propose to predict the kind of redis operations that will be performed by the app code during db.NewDB() call itself -- which will done by the translib infra?

Are you planning to use IsWriteDisabled flag? If yes, please mention so instead of listing redis operations.


Additionally go-redis has internal counters for Connection Pools that can be accessed for each Redis Client in the cache. More details are provided [here](https://pkg.go.dev/github.com/go-redis/redis/[email protected]/internal/pool#Stats).

The counters can be fetched through another function provided by RCM, called RedisClientManagerCounters().
Copy link
Collaborator

Choose a reason for hiding this comment

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

Consider integrating with the existing stats APIs -- https://github.com/sonic-net/sonic-mgmt-common/blob/master/translib/db/db_stats.go


#### Refactoring Callsites

In all places where a Redis Client is not used to perform a transactional operation, the call will be refactored to use RedisClient(). This will result in the caller now using a shared Redis Client with a connection pool. This will significantly reduce the number of new Redis Clients and connections to Redis.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please list the callsites you are planning to change.. Today the DB object and its redis client are created once per translib API call and not by the apps. How the "kind of redis operations" performed by apps are predicted by the infra?


### Restrictions/Limitations

RCM lives in the translib/db package. This package imports CVL, so refactoring calls in CVL to use RCM would create a circular dependency.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can be resolved by placing the RCM code in a separate package parallel to translib & cvl

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.

2 participants