MultiDbClient implementation #3696
Conversation
…v-multi-db-client
| if database_config.client_kwargs.get("retry", None) is not None: | ||
| # The retry object is not used in the lower level clients, so we can safely remove it. | ||
| # We rely on command_retry in terms of global retries. | ||
| database_config.client_kwargs.update({"retry": Retry(retries=0, backoff=NoBackoff())}) |
There was a problem hiding this comment.
I think if the user has provided concrete retry configuration we should keep it and add zero retry config only when the retry is not in the kwargs.
There was a problem hiding this comment.
In my opinion this nesting level of retries creates incorrect expectations, as inner level retries are not registered by FailureDetector that operate on the level of MultiDbClient. In terms of communication MultiDbClient not that different from usual client, so if it faces the same error there's no point to retry it on different layers.
There was a problem hiding this comment.
Ok. I still have one concern - in the current code, the case when the user has not provided a retry config is not handled and the default config of the client will be used - which is with 3 retries.
There was a problem hiding this comment.
Not sure if I get you, we always rely on command_retry parameter which is required and has some default values. The default values is what you concerned?
There was a problem hiding this comment.
The command_retry is used for multi-DB client configuration and the command execution in the context of eventually changing different Redis DB instances.
Here, each database_config contains the configuration for each separate Redis DB instance - each such configuration creates an object of type Redis, RedisCluster, or Sentinel, where we have a default of 3 retries in case retry is not provided in the kwargs. By the way, if a pool is provided directly via from_pool, the retry configuration of the pool is not changed, so in this case, we might end up with unexpected additional retries as well.
Pull Request check-list
Please make sure to review and check all of these items:
NOTE: these things are not required to open a PR and can be done
afterwards / while the PR is open.
Description of change
Added initial implementation of MultiDBClient that allows to operate on Active-Active databases