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

sentinel-cluster-client-redis #1226

Closed
wants to merge 20 commits into from
Closed

sentinel-cluster-client-redis #1226

wants to merge 20 commits into from

Conversation

binecy
Copy link

@binecy binecy commented Dec 26, 2019

Describe what this PR does / why we need it

使用redis作为集群流控的服务端,某些场景下可以减少部署集群流控服务端的工作,也可以充分利用redis的高可用特性。

Does this pull request fix one issue?

Fixes #1188

Describe how you did it

  1. ClusterClientConfigManager管理redis的url,user,password等配置。
  2. RedisProcessorFactoryManager实现了ServerChangeObserver接口,负责构建RedisProcessorFactory和RedisProcessor。RedisProcessor负责与redis交互,向redis请求token,推送rule到redis等。
  3. RedisClusterFlowRuleManager负责将flowRule推送到reids
    flowRule在redis中key为sentinel:config:{namespace-flowId}, 使用Map结构存放sampleCount,intervalInMs,windowLengthInMs,thresholdCount配置。
  4. RedisProcessor.requestToken方法向redis请求token,通过lua脚步实现该功能
    redis中统计数据的key为sentinel:token:{namespace-flowId},使用Map结构存放所有的bucket,Map的key为time/windowLengthInMs,val为pass。
    lua脚步统计数据步骤为:
    使用sentinel:config:{namespace-flowId}查找对应的rule配置
    使用time/windowLengthInMs计算bucket下标
    如果bucket不存在,添加新的bucket同时删除过期的bucket
    统计所有bucket中pass总数,计算qps
    比较结果 添加pass数据

5.RedisClusterFlowSlot处理由RedisClusterFlowRuleManager添加的flowRule,该类代码除了ruleProvider,其他代码与FlowSlot一致。
6.RedisClusterFlowSlotExtender将RedisClusterFlowSlot添加到slotChain

Describe how to verify it

可见ClusterJedisTokenServiceTest,ClusterJedisRuleManagerTest

Special notes for reviews

支持redis4.0+,支持redis单机,sentinel,cluster集群模式,客户端使用jedis。
现在只支持集群总阈值模式,requestToken请求,不支持prioritized参数

单机均摊模式,requestParamToken请求和Lettuce客户端正在开发中

@sczyh30 sczyh30 added area/cluster-flow Issues or PRs related to cluster flow control kind/feature Category issues or prs related to feature request. size/XXL Indicate a PR that changes 1000+ lines. to-review To review labels Dec 26, 2019
@codecov-io
Copy link

codecov-io commented Dec 26, 2019

Codecov Report

Attention: Patch coverage is 0% with 511 lines in your changes missing coverage. Please review.

Project coverage is 41.64%. Comparing base (6a7ec70) to head (61625ab).
Report is 260 commits behind head on master.

Files with missing lines Patch % Lines
...ster/redis/config/RedisClusterFlowRuleManager.java 0.00% 129 Missing ⚠️
...uster/redis/config/ClusterClientConfigManager.java 0.00% 59 Missing ⚠️
...inel/cluster/redis/config/ClusterClientConfig.java 0.00% 52 Missing ⚠️
...libaba/csp/sentinel/cluster/redis/lua/LuaUtil.java 0.00% 49 Missing ⚠️
...p/sentinel/cluster/redis/jedis/JedisProcessor.java 0.00% 37 Missing ⚠️
...ntinel/cluster/redis/RedisClusterTokenService.java 0.00% 32 Missing ⚠️
...ter/redis/config/RedisProcessorFactoryManager.java 0.00% 28 Missing ⚠️
...nel/cluster/redis/jedis/JedisClusterProcessor.java 0.00% 28 Missing ⚠️
...p/sentinel/cluster/redis/RedisClusterFlowSlot.java 0.00% 16 Missing ⚠️
...ster/redis/jedis/JedisClusterProcessorFactory.java 0.00% 16 Missing ⚠️
... and 6 more
Additional details and impacted files
@@             Coverage Diff              @@
##             master    #1226      +/-   ##
============================================
- Coverage     43.64%   41.64%   -2.01%     
+ Complexity     1688     1687       -1     
============================================
  Files           364      380      +16     
  Lines         10523    11034     +511     
  Branches       1416     1466      +50     
============================================
+ Hits           4593     4595       +2     
- Misses         5366     5876     +510     
+ Partials        564      563       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sczyh30
Copy link
Member

sczyh30 commented Jan 15, 2020

Nice, I'll take a review these days. Could you please add some details of the design in the PR description?

@binecy binecy closed this Jan 15, 2020
@binecy
Copy link
Author

binecy commented Jan 15, 2020

谢谢,PR description中已经添加了内容。

@binecy binecy reopened this Jan 15, 2020
@binecy
Copy link
Author

binecy commented Feb 29, 2020

Nice, I'll take a review these days. Could you please add some details of the design in the PR description?

嗨,可以帮忙review一下代码吗
该功能后面还想继续做一些开发,希望你可以先帮忙review一下代码,谢谢

@xiejiashuai
Copy link

xiejiashuai commented Mar 2, 2020

Nice, I'll take a review these days. Could you please add some details of the design in the PR description?

嗨,可以帮忙review一下代码吗
该功能后面还想继续做一些开发,希望你可以先帮忙review一下代码,谢谢

嗨,方便加个钉钉吗?

@sczyh30
Copy link
Member

sczyh30 commented Mar 2, 2020

Nice, I'll take a review these days. Could you please add some details of the design in the PR description?

嗨,可以帮忙review一下代码吗
该功能后面还想继续做一些开发,希望你可以先帮忙review一下代码,谢谢

Sorry for the late reply. We'll take a rapid review these days.

Copy link
Collaborator

@jasonjoo2010 jasonjoo2010 left a comment

Choose a reason for hiding this comment

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

There are some designing issues should be confirmed i think and i need your suggestions @sczyh30 @cdfive

I just left some proposals here and felt it is more complicated as what i expected.

sentinel-cluster/pom.xml Show resolved Hide resolved
@@ -0,0 +1,27 @@
package com.alibaba.csp.sentinel.cluster.redis.config;

Copy link
Collaborator

Choose a reason for hiding this comment

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

And i just want to question that why we created a suit of shadow configuration classes (HostAndPort, Client, Factory) but didn't use that in redisclient? We can handle different types of clients in RedisClientFactory (replacement of RedisClientFactoryManager) originally.

Copy link
Author

Choose a reason for hiding this comment

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

redisclient有歧义,该类负责调用redis执行requestToken等逻辑操作,而不是redis的客户类(已改成RedisProcessor),所以HostAndPort这些配置类没有用于redisclient,而用于负责生成它的RedisClientFactory(已改为RedisProcessorFactory)中,RedisClientFactoryManager(已改成RedisProcessorFactoryManager)则根据不同redis java客户端和redis集群方式生成对应的RedisClientFactory。你看这样是否合理


import java.util.Set;

public interface RedisClient {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the loading process should not be put in client (or we call it proxy or util) and load when needed. This may cause racing or latency, right?

Better put them in kinds of initializing codes like InitFunc, etc

And the name RedisClient will cause ambiguous with real client of Redis.

Copy link
Author

Choose a reason for hiding this comment

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

redis加载工作确实在RedisClusterClientInitFunc#initJedisClient方法中RedisProcessorFactoryManager.setClientType完成。
RedisClient已改为RedisProcessor。


@Override
public int requestToken(String luaId, RequestData requestData) {
final String flowId = String.valueOf(requestData.getFlowId());
Copy link
Collaborator

Choose a reason for hiding this comment

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

How to separate the flowId in different projects sharing same redis/redis cluster?

Copy link
Author

Choose a reason for hiding this comment

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

添加了namespace,ClusterFlowRuleManager为flowRule指定所属的namespace,可以区分不同项目的flowId。

@Override
public int requestToken(String luaId, RequestData requestData) {
String luaCode = LuaUtil.loadLuaCodeIfNeed(luaId);
String luaSha = LuaUtil.loadLuaShaIfNeed(luaCode, new Function<String, String>() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Have you tested the scenario migration of redis cluster? Is the lua cache migrated or not? If not there it would fail then.

Copy link
Author

Choose a reason for hiding this comment

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

redis cluster迁移到新集群,要通过ClusterClientConfigManager.applyNewConfig更新配置,重新创建redis连接,这时也会清理所有的lua sha缓存

@@ -0,0 +1,125 @@
package com.alibaba.csp.sentinel.cluster.redis.config;
Copy link
Collaborator

Choose a reason for hiding this comment

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

And all new files should prepend the license header please.

Copy link
Author

Choose a reason for hiding this comment

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

不好意思,不太理解这个问题,可以再解释一下吗

@CLAassistant
Copy link

CLAassistant commented Mar 4, 2020

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

travis-ci.org err fix
@binecy binecy closed this Mar 5, 2020
@binecy binecy reopened this Mar 5, 2020
@binecy
Copy link
Author

binecy commented Mar 5, 2020

Nice, I'll take a review these days. Could you please add some details of the design in the PR description?

嗨,可以帮忙review一下代码吗
该功能后面还想继续做一些开发,希望你可以先帮忙review一下代码,谢谢

嗨,方便加个钉钉吗?

Nice, I'll take a review these days. Could you please add some details of the design in the PR description?

嗨,可以帮忙review一下代码吗
该功能后面还想继续做一些开发,希望你可以先帮忙review一下代码,谢谢

嗨,方便加个钉钉吗?

钉钉binecy,欢迎交流

@binecy binecy requested a review from jasonjoo2010 March 21, 2020 00:34
@sczyh30
Copy link
Member

sczyh30 commented Mar 26, 2020

Please pay attention to #411 and #1308, where the design of slot and slot chain SPI has been refactored.

@binecy
Copy link
Author

binecy commented Apr 12, 2020

Please pay attention to #411 and #1308, where the design of slot and slot chain SPI has been refactored.

已合并master代码,并使用新的spi机制

@binecy binecy closed this Oct 14, 2021
@Neil-Boyle
Copy link

anything update?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/cluster-flow Issues or PRs related to cluster flow control kind/feature Category issues or prs related to feature request. size/XXL Indicate a PR that changes 1000+ lines. to-review To review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Redis-based token client implementation of cluster flow control
7 participants