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: add sequencer api component by mysql #509

Closed
wants to merge 23 commits into from
Closed

feature: add sequencer api component by mysql #509

wants to merge 23 commits into from

Conversation

GimmeCyy
Copy link
Contributor

What this PR does:
add sequencer api component by mysql

Which issue(s) this PR fixes:
#489

@codecov
Copy link

codecov bot commented Apr 29, 2022

Codecov Report

Merging #509 (d35ec1a) into main (5bada7f) will decrease coverage by 0.36%.
The diff coverage is 39.09%.

@@            Coverage Diff             @@
##             main     #509      +/-   ##
==========================================
- Coverage   60.70%   60.33%   -0.37%     
==========================================
  Files         120      122       +2     
  Lines        6382     6492     +110     
==========================================
+ Hits         3874     3917      +43     
- Misses       2139     2195      +56     
- Partials      369      380      +11     
Impacted Files Coverage Δ
components/pkg/utils/mysql.go 0.00% <0.00%> (ø)
components/sequencer/mysql/mysql.go 54.43% <54.43%> (ø)

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 5bada7f...d35ec1a. Read the comment docs.

@seeflood seeflood added this to the v0.5 milestone May 1, 2022
@seeflood seeflood requested review from a team May 2, 2022 11:35
@seeflood
Copy link
Member

seeflood commented May 5, 2022

@stulzq @ZLBer Hi, could u help review this PR ?

@seeflood
Copy link
Member

seeflood commented May 6, 2022

Currently we can ignore the codecov error in CI because all UT won't be run.

components/sequencer/mysql/mysql.go Outdated Show resolved Hide resolved
components/sequencer/mysql/mysql.go Outdated Show resolved Hide resolved
components/sequencer/mysql/mysql.go Outdated Show resolved Hide resolved
components/sequencer/mysql/mysql.go Outdated Show resolved Hide resolved
components/sequencer/mysql/mysql.go Outdated Show resolved Hide resolved
components/sequencer/mysql/mysql.go Outdated Show resolved Hide resolved
components/sequencer/mysql/mysql.go Outdated Show resolved Hide resolved
components/sequencer/mysql/mysql.go Outdated Show resolved Hide resolved
@seeflood seeflood requested review from wenxuwan and ZLBer May 19, 2022 04:24
@@ -397,6 +398,9 @@ func NewRuntimeGrpcServer(data json.RawMessage, opts ...grpc.ServerOption) (mgrp
runtime_sequencer.NewFactory("in-memory", func() sequencer.Store {
return sequencer_inmemory.NewInMemorySequencer()
}),
runtime_sequencer.NewFactory("Mysql", func() sequencer.Store {
Copy link
Member

Choose a reason for hiding this comment

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

lowercase

)

const (
defaultTableName = "tableName"
Copy link
Member

Choose a reason for hiding this comment

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

Can you change defaultTableName to another? such as layotto_sequencer

}
} else {
Value += 1
_, err := begin.Exec("UPDATE ? SET sequencer_value += 1 WHERE sequencer_key = ?", metadata.TableName, req.Key)
Copy link
Member

Choose a reason for hiding this comment

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

might occur concurrency problem , such as clientA and clientB all want to add the sequencer form 1 to 2 , the db sequencer added to 3,but the both client will get 2.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do I need to use SnowFlake? Or simply add a lock to the method.

Copy link
Member

@ZLBer ZLBer May 27, 2022

Choose a reason for hiding this comment

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

@GimmeCyy global lock? not very good. try db optimistic lock ?


} else {
Value += int64(req.Size)
_, err1 := begin.Exec("UPDATE ? SET sequencer_value = ? WHERE sequencer_key = ?", metadata.TableName, Value, req.Key)
Copy link
Member

Choose a reason for hiding this comment

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

same as above

@LXPWing
Copy link
Member

LXPWing commented May 20, 2022

Why is this pr closed? Is it a wrong click? hhhhh

@LXPWing LXPWing reopened this May 20, 2022
@seeflood
Copy link
Member

seeflood commented May 23, 2022

@GimmeCyy Great contribution. Could u add more test cases to increase ut coverage rate and fix the codecov errors?
image

We have fixed the CI

@GimmeCyy
Copy link
Contributor Author

我的代码推了几次这里都没反应,我可能需要重新提一个pr

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.

5 participants