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

feature: registry add heartbeat #3889

Merged
merged 5 commits into from
Aug 23, 2021
Merged

feature: registry add heartbeat #3889

merged 5 commits into from
Aug 23, 2021

Conversation

xyz327
Copy link
Contributor

@xyz327 xyz327 commented Jul 21, 2021

Ⅰ. Describe what this PR did

add consul redis etcd3 registry heartbeat
#3812

Ⅱ. Does this pull request fix one issue?

Ⅲ. Why don't you add test cases (unit test/integration test)?

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

@codecov-commenter
Copy link

codecov-commenter commented Jul 21, 2021

Codecov Report

Merging #3889 (819cd40) into develop (cfa2b24) will decrease coverage by 0.06%.
The diff coverage is 0.00%.

Impacted file tree graph

@@              Coverage Diff              @@
##             develop    #3889      +/-   ##
=============================================
- Coverage      40.37%   40.30%   -0.07%     
+ Complexity      3073     3070       -3     
=============================================
  Files            686      687       +1     
  Lines          23200    23231      +31     
  Branches        2869     2868       -1     
=============================================
- Hits            9367     9364       -3     
- Misses         12964    12997      +33     
- Partials         869      870       +1     
Impacted Files Coverage Δ
...ery/registry/consul/ConsulRegistryServiceImpl.java 1.70% <0.00%> (-0.07%) ⬇️
...o/seata/discovery/registry/RegistryHeartBeats.java 0.00% <0.00%> (ø)
...covery/registry/etcd3/EtcdRegistryServiceImpl.java 11.45% <0.00%> (-0.18%) ⬇️
...n/src/main/java/io/seata/common/util/IdWorker.java 77.08% <0.00%> (-6.25%) ⬇️

@funky-eyes
Copy link
Contributor

会有连接不自动重连,导致registry的时候失败吗?

}, period, period, TimeUnit.MILLISECONDS);
}

private static long getHeartbeatPeriod(String registryType) {
Copy link
Member

Choose a reason for hiding this comment

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

where HEARTBEAT configuration item ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

where HEARTBEAT configuration item ?

配置直接用 seata 的配置对象 , 使用 registry.{registryType}.heartbeat.period registry.{registryType}.heartbeat.enabled 来配置心跳间隔和是否启动

@suhli
Copy link
Contributor

suhli commented Jul 30, 2021

为啥不在server那层加这个heartbeat?

@xyz327
Copy link
Contributor Author

xyz327 commented Aug 2, 2021

会有连接不自动重连,导致registry的时候失败吗?

遇到的问题场景是我们使用 consul 做注册中心(没有持久化),极端情况导致整个 consul 集群重启,然后所有的 seata 节点注册信息就没有了.然后这时 seata 也不会重试注册(除非重启 seata 节点).

@xyz327
Copy link
Contributor Author

xyz327 commented Aug 2, 2021

为啥不在server那层加这个heartbeat?

在注册中心的 server 层做?

@suhli
Copy link
Contributor

suhli commented Aug 3, 2021

为啥不在server那层加这个heartbeat?

在注册中心实现的 server 层?

对啊,还有是不是没考虑到unregister的时候的情况?像redis unregister的时候应该要停掉的吧?

@xyz327
Copy link
Contributor Author

xyz327 commented Aug 3, 2021

为啥不在server那层加这个heartbeat?

在注册中心实现的 server 层?

对啊,还有是不是没考虑到unregister的时候的情况?像redis unregister的时候应该要停掉的吧?

例如在 redis/etcd 做这个心跳机制并不合适吧,毕竟他们只是个 k-v 存储而已,而且也不能统一(难度大 🤣). unregister 这个场景确定是没有考虑到, 需要处理下, 没想到什么场景下会做 unregister 的动作.

@funky-eyes
Copy link
Contributor

请在changes 文件夹中1.5.0的两个md文件登记下pr和作者信息

@suhli
Copy link
Contributor

suhli commented Aug 9, 2021

为啥不在server那层加这个heartbeat?

在注册中心实现的 server 层?

对啊,还有是不是没考虑到unregister的时候的情况?像redis unregister的时候应该要停掉的吧?

例如在 redis/etcd 做这个心跳机制并不合适吧,毕竟他们只是个 k-v 存储而已,而且也不能统一(难度大 🤣). unregister 这个场景确定是没有考虑到, 需要处理下, 没想到什么场景下会做 unregister 的动作.

像redis这种在key被误删或者redis被强制重启的情况啊,不过这种情况有点极端,另外unregister这个应该要看具体的实现

@suhli
Copy link
Contributor

suhli commented Aug 9, 2021

为啥不在server那层加这个heartbeat?

在注册中心实现的 server 层?

对啊,还有是不是没考虑到unregister的时候的情况?像redis unregister的时候应该要停掉的吧?

例如在 redis/etcd 做这个心跳机制并不合适吧,毕竟他们只是个 k-v 存储而已,而且也不能统一(难度大 🤣). unregister 这个场景确定是没有考虑到, 需要处理下, 没想到什么场景下会做 unregister 的动作.

像redis这种在key被误删或者redis被强制重启的情况啊,不过这种情况有点极端,另外unregister这个应该要看具体的discovery client

对了像redis这些注册的时候会广播事件的是不是要做一些处理?

@funky-eyes funky-eyes changed the title feat: registry add heartbeat feature: registry add heartbeat Aug 21, 2021
Copy link
Contributor

@funky-eyes funky-eyes left a comment

Choose a reason for hiding this comment

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

LGTM @xingfudeshi PTAL

@xingfudeshi xingfudeshi self-requested a review August 23, 2021 03:32
Copy link
Member

@xingfudeshi xingfudeshi left a comment

Choose a reason for hiding this comment

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

LGTM.

@funky-eyes funky-eyes added this to the 1.5.0 milestone Aug 23, 2021
@funky-eyes funky-eyes merged commit 6b45e42 into apache:develop Aug 23, 2021
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.

None yet

6 participants