unified stub and cluster configuration #65 #90

OhBonsai · 2020-07-03T12:53:42Z

Describe what this PR does / why we need it

Unified redis single client and cluster client configuration

Does this pull request fix one issue?

Fixes #65

Describe how you did it

rename redis.go to stub.go, redis_test.go to stub_test.go
add interface Redis and RedisConfig
make RedisStub and RedisClusterStub implement interface Redis
add function RawRedisConfig unified cluster and single configuration
compatible with past configuration

Describe how to verify it

example/client/redis/main.go run: go run main.go -config=config.toml

Special notes for reviews

maybe we can clean old config if compatibility is not so important
remember to update doc, or leave it to me

mlboy · 2020-07-04T01:03:11Z

please rebase your change in a commit.

mlboy

讨论是否从redis.Client、redis.ClusterClient 层抽象接口？
将公共指令统一抽象成 XClient 接口（即现有redis_cmd.go/cluster_cmd.go 的cmd都是公共指令）
差异部分，可根据需要通过断言 client.(type) 获得具体类型

mlboy · 2020-07-04T05:39:39Z

pkg/client/redis/redis.go

+var _ Redis = (*RedisClusterStub)(nil)
+var _ Redis = (*RedisStub)(nil)
+
+func StdRedisConfig(name string) RedisConfig {


config 相关的方法是否考虑仍然放在config.go文件中？

Ok, redis.go文件只留一个Redis interface{}

mlboy · 2020-07-04T05:56:34Z

pkg/client/redis/redis.go

-func newRedisStub(config *RedisConfig) *RedisStub {
-	stub := &RedisStub{
-		conf: config,
+func RawRedisConfig(key string) RedisConfig {


考虑配置文件采用下述格式更加显式：

[jupiter.redis.myredis] addrs = ["one"]

return RedisStub

[jupiter.redis.myredis] addrs = ["one","more"]

return RedisCluster

并且增加显示的指定用法

[jupiter.redis.myredis] addrs = ["one"] client = cluster

return RedisCluster

client = redis/cluster

我认为叫client不准确，不如改为mode, redis后续应该要支持sentinel模式，所以mode可以为stub, cluster, sentinel三种

现阶段配置->结构的Map

addrs.length == 0 => panic log

addrs.length == 1 & (mode == ''" | mode == "stub") => return stubClient

addrs.length == 1 & mode == "cluster" => warning log, return clusterClient

addrs.length > 1 & mode == "stub" => warning log, return stubClient

addrs.length > 1 & mode == "cluster" => return clusterClient

👌我认为可以。

addrs.length > 1 & mode == "" => return clusterClient

OhBonsai · 2020-07-04T07:03:47Z

讨论是否从redis.Client、redis.ClusterClient 层抽象接口？
将公共指令统一抽象成 XClient 接口（即现有redis_cmd.go/cluster_cmd.go 的cmd都是公共指令）
差异部分，可根据需要通过断言 client.(type) 获得具体类型

以接口方式提供缓存服务肯定是应该的，不过断言让使用者自己拿到结构体，然后调用方法，这样无法解决统一生产/测试，集群/单节点的矛盾，而是将这个问题抛给使用者进行运行时判断，这会带来两个问题

让使用者逻辑复杂，要使用独有方法必须要断言。
当用户使用的是interface{} Redis, 他不一定知道有(stub.cluster, sentinel)这些具体类型存在。然后就觉这个client太傻比了，这个简单方法都没有

我觉得比较好的处理方式是， interface定义好所有方法，如果遇到特定方法, 使用给一个warning log, 然后不做任何事情

type Redis interface {
	ClusterFunc() 
}

func (rs *RedisStub) ClusterFunc() {
	logger.warning("you're calling an cluster only function")
}

mlboy · 2020-07-04T14:22:20Z

讨论是否从redis.Client、redis.ClusterClient 层抽象接口？
将公共指令统一抽象成 XClient 接口（即现有redis_cmd.go/cluster_cmd.go 的cmd都是公共指令）
差异部分，可根据需要通过断言 client.(type) 获得具体类型

以接口方式提供缓存服务肯定是应该的，不过断言让使用者自己拿到结构体，然后调用方法，这样无法解决统一生产/测试，集群/单节点的矛盾，而是将这个问题抛给使用者进行运行时判断，这会带来两个问题

让使用者逻辑复杂，要使用独有方法必须要断言。

当用户使用的是interface{} Redis, 他不一定知道有(stub.cluster, sentinel)这些具体类型存在。然后就觉这个client太傻比了，这个简单方法都没有

我觉得比较好的处理方式是， interface定义好所有方法，如果遇到特定方法, 使用给一个warning log, 然后不做任何事情
type Redis interface {
	ClusterFunc() 
}

func (rs *RedisStub) ClusterFunc() {
	logger.warning("you're calling an cluster only function")
}

像下面这样将接口抽象在内部，对用户暴露还是一个结构体。

//Jupiter's Redis
package redis

type IClient interface {
          Get(key string) string 
          Set(...) ...
          ... common method ...  redis.Client、redis.ClusterClient  eg.  
}
type Client struct {
	conf   *RedisConfig
	Client IClient
}
func (c *Client) Get(key string) string {
  return c.Client.Get(key)
}

func (c *Client) Cluster() (*redis.ClusterClient) {
   if cluster,ok := c.Client.(*redis.ClusterClient); !ok {
        return nil
   }
   return cluster
}

//若采用你提到的补充空的 ClusterFunc系列方法，即另外增如下系列的方法：

func (c *Client) ClusterFunc() {
   if cc := c.Cluster() ;cc != nil {
        return cc.ClusterFunc() // 存在就返回
   }
   nop := fakeCluster() //不存在就构造空操作的， 并在fakeCluster 中定义具体记日志的细节
   return nop.ClusterFunc()//返回空操作
}

OhBonsai · 2020-07-05T05:24:26Z

像下面这样将接口抽象在内部，对用户暴露还是一个结构体。

Hello , 想确认一下对用户暴露struct和对用户暴露interface 的好处在哪里？我的理解是能够直接通过.Config来得到当前redis的配置。但是您在这里写的config是私有变量呀？这种对外输出的方式是有规范？还是处于兼容性考虑？

我接触golang时间不久，可能会问一些白痴的问题，请谅解😄

因为我现在比较暴露interface和暴露struct的两种形式的实现方法

两种方法现阶段实现的功能是一致的
interface实现的代码量少
interface更加符合通用的一个哲学，即对外输出一个包，使用该包的人不关心内存的布局(结构)，而关心包提供的方法的集合(接口)

mlboy · 2020-07-05T11:07:45Z

像下面这样将接口抽象在内部，对用户暴露还是一个结构体。

Hello , 想确认一下对用户暴露struct和对用户暴露interface 的好处在哪里？我的理解是能够直接通过.Config来得到当前redis的配置。但是您在这里写的config是私有变量呀？这种对外输出的方式是有规范？还是处于兼容性考虑？

我接触golang时间不久，可能会问一些白痴的问题，请谅解😄

因为我现在比较暴露interface和暴露struct的两种形式的实现方法

两种方法现阶段实现的功能是一致的

interface实现的代码量少

interface更加符合通用的一个哲学，即_对外输出一个包，使用该包的人不关心内存的布局(结构)，而关心包提供的方法的集合(接口)_

抱歉，我这里只是写了下伪代码，用来表达一下我的思路，没考虑具体变量的可读性等细节。

暴露api是用struct 还是interface，肯定是interface 更灵活、也更符合interface的作用。

我描述中的 Client结构体是原stub_cmds.go 和 cluster_cmd.go 中的具体实现细节。

（stub_cmds.go、cluster_cmd.go 代码其实是重复的，只是调用的内部对象 Client 分别是 redis. Client 和redis.ClusterClient）

所以，也就是我上面实现应该是代码最少的。

加了一点注释

//自定义redis包
package redis

//IClient 实现  redis.Client、redis.ClusterClient  的公共方法，即主要是普通操类方法
//这个接口主要屏蔽掉  redis.Client、redis.ClusterClient  的差异
type IClient interface {
          Get(key string) string 
          ....
}

//原来 reids_stub  redis_cluster sturct  合成了Client struct.
type Client struct {
	Config   *RedisConfig
	Client IClient //这里在初始化时候  会根据情况 赋值redis.Client 或redis.ClusterClient
}
//这里具体是包装原有方法的细节...
func (c *Client) Get(key string) string {
  return c.Client.Get(key)
}

func (c *Client) Cluster() (*redis.ClusterClient) {
   if cluster,ok := c.Client.(*redis.ClusterClient); !ok {
        return nil
   }
   return cluster
}
//若想通过interface方式对用户暴露api

//抽象完成的对用户暴露的api
type IRedis interface {
          Get(key string) string 
          Cluster() (*redis.ClusterClient) 
          ....
}

上面可能描述的还是有点模糊。

底层： redis.Client、redis.ClusterClient
底层接口抽象: IClient
包装层: Client
包装接口抽象: IRedis

调用链路是：（称之：B+方案）
IRedis---> Client---> IClient --->(redis.Client | redis.ClusterClient )
接口 ---> 具体包装实现 ---> 公共接口 ---> 具体redis底层类

（因为仅是伪代码描述，名称起的有点乱，可能具体还要斟酌，其中带I的表示接口，否则是具体实现）

另：之前描述，没有 IRedis提到这个接口。个人感觉这里可以不必做多余抽象成接口，直接暴露 Client struct 即可。

即最简链路：（称之：B方案）
Client---> IClient --->(redis.Client | redis.ClusterClient )
具体包装实现 ---> 公共接口 ---> 具体redis底层类

现有链路：(称之：A方案）

Redis---> RedisStub ---> redis.Client
---> RedisCluster ---> redis.ClusterClient
接口 ---> 具体包装实现 ---> 具体redis底层类
---> 具体包装实现 ---> 具体redis底层类

对比差异：
A方案:链路分明，但具体包装部分实现两遍相对重复内容
B方案:首先对底层redis类进行了屏蔽细节
B+方案：对外暴露接口，但相对来说多余

以上仅是我的观点，仅先做讨论。

另外 warning log 的方式，@askuy 表示不建议这么做。

askuy · 2020-07-06T03:50:48Z

@OhBonsai
warning log，可能导致线上功能，线下无法测试。这个对于业务方是有影响的。
建议切换这个功能，只针对redis通用功能，切换普通redis或者分片redis。如果要使用redis 分片的能力，需要显式指定它为分片redis

OhBonsai · 2020-07-06T09:28:00Z

@OhBonsai
warning log，可能导致线上功能，线下无法测试。这个对于业务方是有影响的。
建议切换这个功能，只针对redis通用功能，切换普通redis或者分片redis。如果要使用redis 分片的能力，需要显式指定它为分片redis

这点我没考虑到，从变更安全的角度，开发者使用Cluster专有函数，应该要知道自己使用的是Cluster。如果库只给了一个warning log，会把不稳定因素带到生产，把问题推迟到运行时爆发。那不如采用B方案？ B+把事情说复杂了，简单点最好

mlboy · 2020-07-07T00:35:05Z

@OhBonsai
warning log，可能导致线上功能，线下无法测试。这个对于业务方是有影响的。
建议切换这个功能，只针对redis通用功能，切换普通redis或者分片redis。如果要使用redis 分片的能力，需要显式指定它为分片redis

这点我没考虑到，从变更安全的角度，开发者使用Cluster专有函数，应该要知道自己使用的是Cluster。如果库只给了一个warning log，会把不稳定因素带到生产，把问题推迟到运行时爆发。那不如采用B方案？ B+把事情说复杂了，简单点最好

我认为可以。另外，感谢你的工作与讨论。

OhBonsai · 2020-07-07T16:15:06Z

@askuy 我按照B方案提交了一版本，这不是最终的提交。所以我没有rebase. 暂时还有两个问题需要确认

兼容性考虑

因为Jupiter暂时还在快速迭代中，暂时应该没有项目在生产环境中使用其开源版本把？是否需要兼容StubConfig, ClusterConfig, 我认为在有了RedisConfig之后，功能上不需要StubConfig和ClusterConfig, 这样能节省大概100行代码量。也能给予使用者更少的选择

测试

在单元测试用例中起容器来构建测试所需依赖，会用到https://github.com/ory/dockertest这个库。
预先安装好测试环境，现在的方式。

两者都有好处和坏处

可以让其他开发者，不需要搭建相关环境，直接在ide debug测试。这是一个让新手快速进行单步的方法，毕竟安装一个redis cluster可不是那么容易，缺点是比较麻烦，并且在打包前全量测试会频繁销毁创建容器，让测试变慢
比较简单，同时可以在全量单元测试之前预先安装好测试所需环境，出测试报告快一些

我应该选择那种方式编写redis的测试？

mlboy · 2020-07-08T05:37:29Z

@askuy 我按照B方案提交了一版本，这不是最终的提交。所以我没有rebase. 暂时还有两个问题需要确认

兼容性考虑

因为Jupiter暂时还在快速迭代中，暂时应该没有项目在生产环境中使用其开源版本把？是否需要兼容StubConfig, ClusterConfig, 我认为在有了RedisConfig之后，功能上不需要StubConfig和ClusterConfig, 这样能节省大概100行代码量。也能给予使用者更少的选择

测试

在单元测试用例中起容器来构建测试所需依赖，会用到https://github.com/ory/dockertest这个库。

预先安装好测试环境，现在的方式。

两者都有好处和坏处

可以让其他开发者，不需要搭建相关环境，直接在ide debug测试。这是一个让新手快速进行单步的方法，毕竟安装一个redis cluster可不是那么容易，缺点是比较麻烦，并且在打包前全量测试会频繁销毁创建容器，让测试变慢

比较简单，同时可以在全量单元测试之前预先安装好测试所需环境，出测试报告快一些

我应该选择那种方式编写redis的测试？

我认为增加自动创建本地容器构建测试环境是非常cool的事情。
这使得贡献者可以快速参与贡献，快速完成测试，从而提供更高质量的代码。

另外两种测试并不存在矛盾。即可以同时提供两种方案：

直接跑测试用例 make test
通过自动构建测试环境在容器内完成跑测试用例 make testindocker

最后这些『最佳实践』可以写成文档，放在readme的如何贡献代码部分。

另外我现在使用的是一种偷懒的策略，去跑那些需要环境支持的测试用例，即：
通过fork repo后，push 改动到自己仓库，触发自己仓库的actions构建，通过它去看测试报告。

关于https://github.com/ory/dockertest 我没有使用过，不了解机制，需要确认几个点：

它创建的临时环境端口占用，是否会影响开发者现有容器。（即达到不占用宿主机端口）
它的引入是否需要侵入jupiter 的go.mod 依赖（即希望 dockertest的引入可以保持独立，做的jupiter 本身不依赖dockertest项目）

StubConfig和ClusterConfig, 合并的问题，涉及对用户配置规范暴露，需要 @askuy 来决定。

考虑后续增加版本内容规划、Jupiter版本号的规划，以处理非兼容改动取舍的问题。

OhBonsai · 2020-07-08T15:24:48Z

@mlboy dockertest实际上就是docker官方库包装了一层，方便在测试的时候使用。

对于你关切的两个问题

端口占用：会! 但可以通过publish一个不常用的端口来避免这个问题
依赖侵入: 会！不过好像go.mod 没有test-requirement only的概念，官方有一些冗长的讨论, 根据go mod的issue。会更新go.mod 但是code 会lazy download，只有在运行go test的之后才会下载代码。不知是否能够接受？

我加了一个case, 如果确认OK，我将用这种方法补充完成Redis Struct的测试

mlboy · 2020-07-11T06:29:07Z

@mlboy dockertest实际上就是docker官方库包装了一层，方便在测试的时候使用。

对于你关切的两个问题

端口占用：会! 但可以通过publish一个不常用的端口来避免这个问题

依赖侵入: 会！不过好像go.mod 没有test-requirement only的概念，官方有一些冗长的讨论, 根据go mod的issue。会更新go.mod 但是code 会lazy download，只有在运行go test的之后才会下载代码。不知是否能够接受？

我加了一个case, 如果确认OK，我将用这种方法补充完成Redis Struct的测试

引入 dockertest 还有个问题，普通开发没有docker环境时候还能否继续继续执行测试？
即是不是会将docker变成必须依赖的开发环境。

还有既然有了 Redis Struct 里面的方法实现，为什么在 stub_cmds.go 中还有一遍相同的实现？

我发了一封邮件到到你在github个人主页所留的邮箱中，请查收。

mlboy

+2

mlboy

thanks for your job

unified stub and cluster configuration douyu#65

OhBonsai force-pushed the master branch from e2eec69 to 8fcc5f1 Compare July 4, 2020 01:56

mlboy reviewed Jul 4, 2020

View reviewed changes

OhBonsai changed the title ~~unified stub and cluster configuration #65~~ WIP: unified stub and cluster configuration #65 Jul 7, 2020

OhBonsai force-pushed the master branch from aff14de to 6941f34 Compare July 14, 2020 03:59

OhBonsai changed the title ~~WIP: unified stub and cluster configuration #65~~ unified stub and cluster configuration #65 Jul 14, 2020

OhBonsai force-pushed the master branch from 6941f34 to c1d1da1 Compare July 14, 2020 08:32

mlboy reviewed Jul 14, 2020

View reviewed changes

refactor(redis): unified stud and cluster configuration

7c9d6ef

OhBonsai force-pushed the master branch from c1d1da1 to 7c9d6ef Compare July 14, 2020 13:59

mlboy reviewed Jul 14, 2020

View reviewed changes

mlboy merged commit b7a0ee8 into douyu:master Jul 14, 2020

achillesss pushed a commit to achillesss/jupiter that referenced this pull request Oct 27, 2020

Merge pull request douyu#90 from OhBonsai/master

79a1035

unified stub and cluster configuration douyu#65

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

unified stub and cluster configuration #65 #90

unified stub and cluster configuration #65 #90

OhBonsai commented Jul 3, 2020

mlboy commented Jul 4, 2020

mlboy left a comment •

edited

Loading

mlboy Jul 4, 2020

OhBonsai Jul 4, 2020

mlboy Jul 4, 2020 •

edited

Loading

OhBonsai Jul 4, 2020

mlboy Jul 4, 2020

OhBonsai commented Jul 4, 2020

mlboy commented Jul 4, 2020 •

edited

Loading

OhBonsai commented Jul 5, 2020 •

edited

Loading

mlboy commented Jul 5, 2020 •

edited

Loading

askuy commented Jul 6, 2020

OhBonsai commented Jul 6, 2020

mlboy commented Jul 7, 2020

OhBonsai commented Jul 7, 2020 •

edited by mlboy

Loading

mlboy commented Jul 8, 2020 •

edited

Loading

兼容性考虑

测试

OhBonsai commented Jul 8, 2020 •

edited

Loading

mlboy commented Jul 11, 2020

mlboy left a comment •

edited

Loading

mlboy left a comment

unified stub and cluster configuration #65 #90

unified stub and cluster configuration #65 #90

Conversation

OhBonsai commented Jul 3, 2020

Describe what this PR does / why we need it

Does this pull request fix one issue?

Describe how you did it

Describe how to verify it

Special notes for reviews

mlboy commented Jul 4, 2020

mlboy left a comment • edited Loading

Choose a reason for hiding this comment

mlboy Jul 4, 2020

Choose a reason for hiding this comment

OhBonsai Jul 4, 2020

Choose a reason for hiding this comment

mlboy Jul 4, 2020 • edited Loading

Choose a reason for hiding this comment

OhBonsai Jul 4, 2020

Choose a reason for hiding this comment

mlboy Jul 4, 2020

Choose a reason for hiding this comment

OhBonsai commented Jul 4, 2020

mlboy commented Jul 4, 2020 • edited Loading

OhBonsai commented Jul 5, 2020 • edited Loading

mlboy commented Jul 5, 2020 • edited Loading

askuy commented Jul 6, 2020

OhBonsai commented Jul 6, 2020

mlboy commented Jul 7, 2020

OhBonsai commented Jul 7, 2020 • edited by mlboy Loading

兼容性考虑

测试

mlboy commented Jul 8, 2020 • edited Loading

兼容性考虑

测试

OhBonsai commented Jul 8, 2020 • edited Loading

mlboy commented Jul 11, 2020

mlboy left a comment • edited Loading

Choose a reason for hiding this comment

mlboy left a comment

Choose a reason for hiding this comment

mlboy left a comment •

edited

Loading

mlboy Jul 4, 2020 •

edited

Loading

mlboy commented Jul 4, 2020 •

edited

Loading

OhBonsai commented Jul 5, 2020 •

edited

Loading

mlboy commented Jul 5, 2020 •

edited

Loading

OhBonsai commented Jul 7, 2020 •

edited by mlboy

Loading

mlboy commented Jul 8, 2020 •

edited

Loading

OhBonsai commented Jul 8, 2020 •

edited

Loading

mlboy left a comment •

edited

Loading