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

sequencer zookeeper #156

Merged
merged 12 commits into from
Aug 12, 2021
Merged

sequencer zookeeper #156

merged 12 commits into from
Aug 12, 2021

Conversation

ZLBer
Copy link
Member

@ZLBer ZLBer commented Jul 28, 2021

What this PR does:
add sequencer zookeeper
Which issue(s) this PR fixes:

Fixes #150

Special notes for your reviewer:

Does this PR introduce a user-facing change?:


@ZLBer
Copy link
Member Author

ZLBer commented Jul 28, 2021

有个问题,zk的version只支持到int32

@codecov-commenter
Copy link

codecov-commenter commented Jul 30, 2021

Codecov Report

Merging #156 (ff61b24) into main (9a0cdbf) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #156   +/-   ##
=======================================
  Coverage   58.10%   58.10%           
=======================================
  Files          50       50           
  Lines        2148     2148           
=======================================
  Hits         1248     1248           
  Misses        778      778           
  Partials      122      122           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c978116...ff61b24. Read the comment docs.

Copy link
Member

@seeflood seeflood left a comment

Choose a reason for hiding this comment

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

有个问题,zk的version只支持到int32

我刚才请教了同事,他说很多公司现在在把id从32位升级到64位,他之前在美团某业务就遇到了id快溢出的问题……
所以我们就不考虑把API改成int32了
那么解决这个问题的方案:
a. zk在组件文档上写个warning:本组件最大2的31次方,超过会溢出
b. 想办法用其他api实现这个功能

关于b方案,我查了一下文档,java的curator是通过value值做的自增id,具体来说,先乐观锁执行get--add--set,如果set发现冲突再重试/升级成悲观锁。见http://curator.apache.org/curator-recipes/distributed-atomic-long.html

考虑到curator没有go版本,自己写的话太复杂了,我们先选择a方案吧,zk就只支持到int32,在文档里写上warning

components/sequencer/zookeeper/zookeeper_sequencer.go Outdated Show resolved Hide resolved
@ZLBer
Copy link
Member Author

ZLBer commented Aug 5, 2021

我刚才请教了同事,他说很多公司现在在把id从32位升级到64位,他之前在美团某业务就遇到了id快溢出的问题……
所以我们就不考虑把API改成int32了
那么解决这个问题的方案:
a. zk在组件文档上写个warning:本组件最大2的31次方,超过会溢出
b. 想办法用其他api实现这个功能

关于b方案,我查了一下文档,java的curator是通过value值做的自增id,具体来说,先乐观锁执行get--add--set,如果set发现冲突再重试/升级成悲观锁。见http://curator.apache.org/curator-recipes/distributed-atomic-long.html

考虑到curator没有go版本,自己写的话太复杂了,我们先选择a方案吧,zk就只支持到int32,在文档里写上warning

@seeflood get ! 我晚点改一下

@ZLBer
Copy link
Member Author

ZLBer commented Aug 6, 2021

@seeflood a commit for above conversation

@seeflood
Copy link
Member

seeflood commented Aug 10, 2021

@ZLBer 我改了一下你的分支,你review下没问题我就合并这个pr了~
改动包括:

  1. GetNextId的时候,从判断==0 改成了判断<=0
  2. 修改中英文文档中的描述

@ZLBer
Copy link
Member Author

ZLBer commented Aug 12, 2021

@seeflood LGTM

Copy link
Member

@seeflood seeflood left a comment

Choose a reason for hiding this comment

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

LGTM

@seeflood seeflood merged commit fff4d99 into mosn:main Aug 12, 2021
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.

Implement Sequencer API component: zk & standalone redis
3 participants