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 redis #152

Merged
merged 10 commits into from
Aug 5, 2021
Merged

sequencer redis #152

merged 10 commits into from
Aug 5, 2021

Conversation

ZLBer
Copy link
Member

@ZLBer ZLBer commented Jul 24, 2021

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

Fixes #150

Special notes for your reviewer:
@seeflood
Does this PR introduce a user-facing change?:


@codecov-commenter
Copy link

codecov-commenter commented Jul 25, 2021

Codecov Report

Merging #152 (575634f) into main (407b55a) will decrease coverage by 0.11%.
The diff coverage is n/a.

❗ Current head 575634f differs from pull request most recent head 5b99774. Consider uploading reports for the commit 5b99774 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main     #152      +/-   ##
==========================================
- Coverage   45.73%   45.62%   -0.12%     
==========================================
  Files          46       46              
  Lines        1725     1725              
==========================================
- Hits          789      787       -2     
- Misses        886      887       +1     
- Partials       50       51       +1     
Impacted Files Coverage Δ
...kg/filter/network/tcpcopy/persistence/work_pool.go 86.27% <0.00%> (-3.93%) ⬇️

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 407b55a...5b99774. Read the comment docs.

@seeflood
Copy link
Member

ZLBer同学太强了 👍 👍 👍

@seeflood
Copy link
Member

seeflood commented Jul 26, 2021

另外看代码的时候突然想到个问题,记录下:
比如用户的state、lock、sequencer API都用redis,那么key可能有冲突
这个问题可以在runtime层做下改动,比如自动给key带上前缀lock||appid||key1 ,sequencer||appid||key1这样。
组件不用管这个事情,我提个pr改下runtime吧

@ZLBer
Copy link
Member Author

ZLBer commented Jul 26, 2021

另外看代码的时候突然想到个问题,记录下:
比如用户的state、lock、sequencer API都用redis,那么key可能有冲突
这个问题可以在runtime层做下改动,比如自动给key带上前缀lock||appid||key1 ,sequencer||appid||key1这样。
组件不用管这个事情,我提个pr改下runtime吧

这个很有必要

@seeflood
Copy link
Member

另外看代码的时候突然想到个问题,记录下:
比如用户的state、lock、sequencer API都用redis,那么key可能有冲突
这个问题可以在runtime层做下改动,比如自动给key带上前缀lock||appid||key1 ,sequencer||appid||key1这样。
组件不用管这个事情,我提个pr改下runtime吧

这个很有必要

恩恩我记了个issue给自己 稍后会改掉 #154

@ZLBer
Copy link
Member Author

ZLBer commented Jul 28, 2021

@seeflood 新的commit修改了上面的问题 并修改了segmentResponse

@ZLBer
Copy link
Member Author

ZLBer commented Jul 30, 2021

@seeflood new commit for above problem

@seeflood
Copy link
Member

seeflood commented Aug 4, 2021

看着没问题了,就是之前我提了个pr,把组件的一些模板代码(比如创建redis和etcd客户端的代码)抽出来成一个公用的工具类了
这个我在你的分支上改下代码吧

@seeflood
Copy link
Member

seeflood commented Aug 4, 2021

另外就是需要在组件配置文档里声明,redis组件可能出现id冲突,为了避免冲突需要使用单机redis、把持久化配置都调到最强

@seeflood
Copy link
Member

seeflood commented Aug 4, 2021

@ZLBer 我给你的分支提交了修改,辛苦你review下哈~包括:

  1. sidebar里加上了文档
  2. 使用了公用的utils方法,删掉冗余代码
  3. 组件配置文档里加入声明,redis组件可能出现id冲突,为了避免冲突需要使用单机redis、把持久化配置都调到最强

如果你review觉得没问题,我就合并这个pr了哈~

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

@ZLBer
Copy link
Member Author

ZLBer commented Aug 5, 2021

@seeflood LGTM

@seeflood seeflood merged commit 5239429 into mosn:main Aug 5, 2021
MoonShining pushed a commit that referenced this pull request Aug 12, 2021
* add request time for rpc

* change totle time

* update

* update

* update

* grpc design docs (#163)

* add trace log

* change log path

* update

* sequencer redis (#152)

* update

* update

* [fix] Sequencer redis (#164)

fix : for GetSegment method, redis component only check support when size=0.

* update

* update

* update

* update

* add unittest for runtime and grpc (#169)

* update

* remove trace time prints

* update

Co-authored-by: 文徐 <wangwenxue.wwx@alibaba-inc.com>
Co-authored-by: 永鹏 <neji_bupt@163.com>
Co-authored-by: ZLBer <1098294815@qq.com>
Co-authored-by: tianjipeng <tianjipeng@outlook.com>
MoonShining pushed a commit that referenced this pull request Aug 12, 2021
* add request time for rpc

* change totle time

* update

* update

* update

* grpc design docs (#163)

* add trace log

* change log path

* update

* sequencer redis (#152)

* update

* update

* [fix] Sequencer redis (#164)

fix : for GetSegment method, redis component only check support when size=0.

* update

* update

* update

* update

* add unittest for runtime and grpc (#169)

* update

* remove trace time prints

* update

Co-authored-by: 文徐 <wangwenxue.wwx@alibaba-inc.com>
Co-authored-by: 永鹏 <neji_bupt@163.com>
Co-authored-by: ZLBer <1098294815@qq.com>
Co-authored-by: tianjipeng <tianjipeng@outlook.com>
MoonShining pushed a commit that referenced this pull request Aug 12, 2021
* add request time for rpc

* change totle time

* update

* update

* update

* grpc design docs (#163)

* add trace log

* change log path

* update

* sequencer redis (#152)

* update

* update

* [fix] Sequencer redis (#164)

fix : for GetSegment method, redis component only check support when size=0.

* update

* update

* update

* update

* add unittest for runtime and grpc (#169)

* update

* remove trace time prints

* update

Co-authored-by: 文徐 <wangwenxue.wwx@alibaba-inc.com>
Co-authored-by: 永鹏 <neji_bupt@163.com>
Co-authored-by: ZLBer <1098294815@qq.com>
Co-authored-by: tianjipeng <tianjipeng@outlook.com>
MoonShining pushed a commit that referenced this pull request Aug 16, 2021
* add request time for rpc

* change totle time

* update

* update

* update

* grpc design docs (#163)

* add trace log

* change log path

* update

* sequencer redis (#152)

* update

* update

* [fix] Sequencer redis (#164)

fix : for GetSegment method, redis component only check support when size=0.

* update

* update

* update

* update

* add unittest for runtime and grpc (#169)

* update

* remove trace time prints

* update

Co-authored-by: 文徐 <wangwenxue.wwx@alibaba-inc.com>
Co-authored-by: 永鹏 <neji_bupt@163.com>
Co-authored-by: ZLBer <1098294815@qq.com>
Co-authored-by: tianjipeng <tianjipeng@outlook.com>
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