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

feat: add redis cluster #938

Merged
merged 13 commits into from
Aug 11, 2023
Merged

Conversation

city55238
Copy link
Contributor

@city55238 city55238 commented Jul 27, 2023

Describe what this PR does / why we need it

redis支持集群模式

Does this pull request fix one issue?

feat

Describe how you did it

在原生redis-go sdk的基础上接入cluster,修改cluster redis配置的加载方式

cluster配置支持云厂商连接:"r-bp1zxszhcgatnx****.redis.rds.aliyuncs.com:6379"

也支持自定义node集群连接: ["127.0.0.1:6379","127.0.0.1:6370","127.0.0.1:6371"]

配置示例:
[jupiter.redis.exampleserver]
[jupiter.redis.exampleserver.cluster]
debug = false
maxIdle = 10
maxActive = 50
dialTimeout = "2s"
readTimeout = "2s"
idleTimeout = "60s"
enableAccessLog = false
addr = ["r-bp1zxszhcgatnx****.redis.rds.aliyuncs.com:6379"] # ["127.0.0.1:6379","127.0.0.1:6370","127.0.0.1:6371"]
username = "root"
password = "xxxxxx"

Describe how to verify it

Special notes for reviews

@city55238 city55238 temporarily deployed to tablestore-live July 27, 2023 11:52 — with GitHub Actions Inactive
@codecov-commenter
Copy link

codecov-commenter commented Jul 27, 2023

Codecov Report

Merging #938 (4a4dd04) into master (6f18fe8) will decrease coverage by 20.15%.
Report is 44 commits behind head on master.
The diff coverage is 0.00%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@             Coverage Diff             @@
##           master     #938       +/-   ##
===========================================
- Coverage   53.16%   33.01%   -20.15%     
===========================================
  Files         202      162       -40     
  Lines       11335     8538     -2797     
===========================================
- Hits         6026     2819     -3207     
- Misses       4857     5486      +629     
+ Partials      452      233      -219     
Files Changed Coverage Δ
pkg/client/redis/cluster.go 0.00% <0.00%> (ø)
pkg/client/redis/cluster_config.go 0.00% <0.00%> (ø)
pkg/client/redis/config.go 47.27% <ø> (-23.64%) ⬇️

... and 98 files with indirect coverage changes

@city55238 city55238 changed the title Feat/add redis cluster feat:add redis cluster Jul 27, 2023
@sysulq
Copy link
Member

sysulq commented Jul 28, 2023

建议新增一种cluster类型,和stub类型同级

@city55238 city55238 temporarily deployed to tablestore-live July 28, 2023 06:10 — with GitHub Actions Inactive
@city55238 city55238 temporarily deployed to tablestore-live July 28, 2023 06:45 — with GitHub Actions Inactive
@sysulq
Copy link
Member

sysulq commented Jul 31, 2023

@city55238 我理解的,是不是把cluster和stub client完全独立会更好呢?

@city55238
Copy link
Contributor Author

redis stub模式也是在第二层的,我理解cluster也应该是在这个位置
image

@city55238 city55238 closed this Aug 4, 2023
@city55238 city55238 reopened this Aug 4, 2023
@sysulq sysulq changed the title feat:add redis cluster feat: add redis cluster Aug 4, 2023
@city55238 city55238 closed this Aug 7, 2023
@city55238 city55238 reopened this Aug 7, 2023
@city55238 city55238 temporarily deployed to tablestore-live August 7, 2023 03:22 — with GitHub Actions Inactive
@sysulq
Copy link
Member

sysulq commented Aug 8, 2023

@city55238 可以参考现有的redis单测,加下cluster的测试用例

@city55238
Copy link
Contributor Author

已经补充过了,但是nomal start的样例需要你那边有一个集群模式的redis,不然单测跑不过

@city55238 city55238 temporarily deployed to tablestore-live August 8, 2023 10:31 — with GitHub Actions Inactive
@city55238 city55238 temporarily deployed to tablestore-live August 9, 2023 02:18 — with GitHub Actions Inactive
@city55238 city55238 temporarily deployed to tablestore-live August 9, 2023 02:37 — with GitHub Actions Inactive
@city55238 city55238 temporarily deployed to tablestore-live August 9, 2023 02:51 — with GitHub Actions Inactive
"github.com/samber/lo"
"go.uber.org/zap"
)

Copy link
Member

Choose a reason for hiding this comment

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

这里如果用组合会不会更简单些

type ClusterClient struct {
	*redis.ClusterClient
	config  *Config
}

config *Config
}

func (ins *ClusterClient) CmdOnCluster() *redis.ClusterClient {
Copy link
Member

Choose a reason for hiding this comment

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

如果是用组合的话,这个方法就不需要了

pkg/client/redis/cluster_test.go Outdated Show resolved Hide resolved
)

type ClusterClient struct {
cluster *redis.ClusterClient
Copy link
Member

Choose a reason for hiding this comment

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

这一行也可以去掉了

pkg/client/redis/config.go Show resolved Hide resolved
@city55238 city55238 temporarily deployed to tablestore-live August 10, 2023 08:51 — with GitHub Actions Inactive
@city55238 city55238 temporarily deployed to tablestore-live August 10, 2023 09:00 — with GitHub Actions Inactive
pkg/client/redis/cluster_config.go Outdated Show resolved Hide resolved
pkg/client/redis/config_test.go Outdated Show resolved Hide resolved
@city55238 city55238 temporarily deployed to tablestore-live August 10, 2023 12:04 — with GitHub Actions Inactive
@sysulq
Copy link
Member

sysulq commented Aug 11, 2023

LGTM

@sysulq sysulq merged commit 7d81c63 into douyu:master Aug 11, 2023
11 checks passed
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