This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@junr03 PTAL

@anpryl what is the use case for this setting?

@junr03 we have single redis instance for multiple applications. Each application uses own database by number.

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

Thanks for the explanation @anpryl. Three high level comments:

1. We need to add documentation for this feature.
2. We need to add tests for this feature.
3. I would like to work to make this feature use a new NewPool method like @repl-david-winiarski did in Redis TLS and Auth support #96. It would be ideal if we don't change the "legacy" plain method. This way we have a precedent for acquiring different types of pools from the driver.

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

Working on it

@junr03

I would like to work to make this feature use a new NewPool method like @repl-david-winiarski did in #96. It would be ideal if we don't change the "legacy" plain method. This way we have a precedent for acquiring different types of pools from the driver.

I'm not sure that I understand you correctly.
Do you propose to add new function like NewPoolImplWithDatabaseNumber instead of extending an existing one?
I think that the database number is very generic connection parameter. Both TLS and non-TLS functions could accept it. In other case we need two version of non-TLS NewPool and two versions for TLS NewPool.
Could you please explain why do we need separate function to accept database number?

@junr03 I reworked NewPoolImpl to accept a list of functions to configure redis client. Also, I add WithDatabase to configure the database number.
Also, it will be easier to extend the connection configuration (for example: TLS).

Working on tests.

@HenryYYang for a second pair of eyes.

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

bump to remove stale tag

I just realized that this is for ratelimit service which use a local copy of redis. I don't have a lot of context on this. That said, multiple database on a single redis is generally a bad thing with little benefit see this discussion. https://groups.google.com/forum/#!topic/redis-db/vS5wX8X4Cjg/discussion
A better alternative would be to change the key to create logical partition within a single database, or create multiple redis instances.

@HenryYYang database numbers is stable redis feature. I believe multiple teams already using it. Why not add support of stable feature?

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

This pull request has been automatically closed because it has not had activity in the last 14 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!