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 consist load balance #600

Merged
merged 234 commits into from
Feb 17, 2024
Merged

Conversation

wang1309
Copy link
Contributor

@wang1309 wang1309 commented Aug 6, 2023

What this PR does:
implement consistent hash func

Which issue(s) this PR fixes:

Fixes #584

Special notes for your reviewer:

Does this PR introduce a user-facing change?:


@codecov-commenter
Copy link

codecov-commenter commented Aug 6, 2023

Codecov Report

Merging #600 (9d4bc1e) into master (97d6a39) will increase coverage by 0.13%.
Report is 1 commits behind head on master.
The diff coverage is 50.56%.

❗ 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     #600      +/-   ##
==========================================
+ Coverage   37.25%   37.38%   +0.13%     
==========================================
  Files         175      176       +1     
  Lines       11659    11755      +96     
==========================================
+ Hits         4343     4395      +52     
- Misses       6952     6992      +40     
- Partials      364      368       +4     
Files Changed Coverage Δ
pkg/client/config.go 62.29% <ø> (ø)
pkg/remoting/getty/rpc_client.go 0.00% <ø> (ø)
pkg/remoting/loadbalance/loadbalance.go 0.00% <0.00%> (ø)
...emoting/loadbalance/consistent_hash_loadbalance.go 51.72% <51.72%> (ø)

... and 1 file with indirect coverage changes

}

func (c *Consistent) put(key int64, session getty.Session) {
c.RLock()
Copy link
Contributor

Choose a reason for hiding this comment

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

这里是不是应该用写锁而不是读锁?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

是,写的时候看错了

return consistentInstance
}

func ConsistentHashLoadBalance(sessions *sync.Map, xid string) getty.Session {
Copy link
Contributor

Choose a reason for hiding this comment

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

目前getty模块目前调用Select时的sessions地址是不变的,所以这里使用单例模式也就没有问题。
不过外部的代码随时可能改变,出于代码健壮性考虑,是不是处理下sessions地址改变的情况会更好?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

突然想起来,我这里有一个 refreshHashCircle 方法,如果 session 地址发生变更,session 相当于 close了,我会重新构造 hashCircle,所以应该能覆盖到这种变更的case

Copy link
Contributor

Choose a reason for hiding this comment

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

@Wang
ConsistentHashLoadBalance实例的创建过程使用了once,当SessionManager管理的sessions *sync.Map地址发生变化,再次调用ConsistentHashLoadBalance,ConsistentHashLoadBalance内部的sessions还是原来的旧地址,新旧地址指向两组完全不同的sessions。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Wang ConsistentHashLoadBalance实例的创建过程使用了once,当SessionManager管理的sessions *sync.Map地址发生变化,再次调用ConsistentHashLoadBalance,ConsistentHashLoadBalance内部的sessions还是原来的旧地址,新旧地址指向两组完全不同的sessions。

对对对,早上还是不够清醒,哈哈

Copy link
Contributor Author

Choose a reason for hiding this comment

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

我在 consistentInstance.pick(sessions, xid) 的时候会把最新的 sessions 传进去,如果检测到无效地址会重新构建单例对象的hash环,应该是cover住了这个场景。

Copy link
Contributor

Choose a reason for hiding this comment

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

外部sessions中的元素可能会增加、减少,pick函数中只检测closed是不够的。

记录一下上次传入的sessions地址,每次被调用时比较下,地址不同就refresh。这个思路你觉得怎么样?

@slievrly slievrly added this to the 1.3.0 milestone Aug 15, 2023
consistentInstance = &Consistent{
hashCircle: make(map[int64]getty.Session),
}
// construct hash circle
Copy link
Contributor

Choose a reason for hiding this comment

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

构建哈希环的这部分代码和refreshHashCircle函数中的高度重合,是不是可以重构下,调用refreshHashCircle函数。

return consistentInstance
}

func ConsistentHashLoadBalance(sessions *sync.Map, xid string) getty.Session {
Copy link
Contributor

Choose a reason for hiding this comment

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

外部sessions中的元素可能会增加、减少,pick函数中只检测closed是不够的。

记录一下上次传入的sessions地址,每次被调用时比较下,地址不同就refresh。这个思路你觉得怎么样?

@luky116
Copy link
Contributor

luky116 commented Feb 3, 2024

已通知代码提交者处理冲突

@luky116 luky116 changed the title Feat consist load balance 【已通知处理】Feat consist load balance Feb 3, 2024
@luky116 luky116 changed the title 【已通知处理】Feat consist load balance 【等CI结束就合并】Feat consist load balance Feb 3, 2024
@wang1309
Copy link
Contributor Author

wang1309 commented Feb 3, 2024

已通知代码提交者处理冲突

看了下没冲突,是你已经解决了吗

@luky116
Copy link
Contributor

luky116 commented Feb 3, 2024

已通知代码提交者处理冲突

看了下没冲突,是你已经解决了吗

是的

@luky116 luky116 changed the title 【等CI结束就合并】Feat consist load balance Feat consist load balance Feb 3, 2024
@luky116 luky116 merged commit e18861d into apache:master Feb 17, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

在remoting模块中实现ConsistentHash负载均衡