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.

Sign up for GitHub

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

Jump to bottom

StateStore API implementation for CockroachDB #1559

Merged
merged 17 commits into from
Mar 22, 2022
Merged

StateStore API implementation for CockroachDB #1559

merged 17 commits into from
Mar 22, 2022

Conversation

r-c-correa
Copy link
Contributor

@r-c-correa r-c-correa commented Mar 7, 2022 •
edited
Loading

Description

This PR introduces a CockroachDB StateStore component. The code is implemented following the example of the PostgreSQL.

Issue reference

Please reference the issue this PR will close: #1556

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

@r-c-correa r-c-correa requested review from a team as code owners March 7, 2022 02:58
@r-c-correa
Copy link
Contributor Author

Next commit I will send Conformance Tests for CockroachDB 😃

r-c-correa and others added 4 commits March 7, 2022 22:13
@r-c-correa
statestore for CockroachDB
322f692 
Signed-off-by: Ricardo Corrêa <[email protected]>
@shubham1172 @r-c-correa
Upgrade nacos sdk version to meet compliance (#1547)
8baa00d 
* Upgrade nacos version

Signed-off-by: Shubham Sharma <[email protected]>

* Run go mod tidy all

Signed-off-by: Shubham Sharma <[email protected]>

* Add MaxSize to settings

Signed-off-by: Shubham Sharma <[email protected]>
Signed-off-by: Ricardo Corrêa <[email protected]>
@r-c-correa
conformance tests for CockroachDB
a4d7f8d 
Signed-off-by: Ricardo Corrêa <[email protected]>
@r-c-correa
Merge branch 'master' into state_cockroachdb
7d51db7
@yaron2
Copy link
Member

yaron2 commented Mar 8, 2022

Really excited to see this! Great contribution and kudos for adding conformance tests.

@r-c-correa r-c-correa mentioned this pull request Mar 8, 2022
Closed
@Taction
Copy link
Member

Taction commented Mar 13, 2022

@r-c-correa Thanks for your contribution! Looks good for me overall, just some small changes.

mukundansundar
mukundansundar reviewed Mar 15, 2022
View reviewed changes
Copy link
Contributor

@mukundansundar mukundansundar left a comment

Choose a reason for hiding this comment

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

@r-c-correa Thanks for the contribution. Added some comments.
Please look at this PR #1550, for conformance with respect to transaction ordering.

state/cockroachdb/cockroachdb.go Outdated Show resolved Hide resolved
state/cockroachdb/cockroachdb.go Show resolved Hide resolved
state/cockroachdb/cockroachdb_access.go Outdated Show resolved Hide resolved
state/cockroachdb/cockroachdb_access.go Outdated Show resolved Hide resolved
state/cockroachdb/cockroachdb_access.go Outdated Show resolved Hide resolved
state/cockroachdb/cockroachdb_access.go Outdated Show resolved Hide resolved
@yaron2
Copy link
Member

yaron2 commented Mar 18, 2022

@r-c-correa ping. address the changes and we can get this merged.

@r-c-correa
Copy link
Contributor Author

@yaron2 I did the changes, I have only one question, waiting for @mukundansundar answer 😄

@yaron2
Copy link
Member

yaron2 commented Mar 21, 2022

@yaron2 I did the changes, I have only one question, waiting for @mukundansundar answer 😄

I don't see any question for @mukundansundar?

@mukundansundar
Copy link
Contributor

mukundansundar commented Mar 21, 2022 •
edited
Loading

Please look at this PR #1550, for conformance with respect to transaction ordering.

@r-c-correa
If this is with respect to the line above ...
Then the transaction order conformance tests pass for cockroachDB ...
https://github.com/dapr/components-contrib/runs/5631808447?check_suite_focus=true#step:33:219

If you have any questions for me, please let me know ....

yaron2
yaron2 previously approved these changes Mar 21, 2022
View reviewed changes
@mukundansundar
Copy link
Contributor

mukundansundar commented Mar 22, 2022 •
edited
Loading

updated doc PR ... dapr/docs#2276

mukundansundar
mukundansundar reviewed Mar 22, 2022
View reviewed changes
Copy link
Contributor

@mukundansundar mukundansundar left a comment

Choose a reason for hiding this comment

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

@r-c-correa small change. otherwise looks good.

state/cockroachdb/cockroachdb_access.go Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Mar 22, 2022

Codecov Report

Merging #1559 (fcab309) into master (34c7eb5) will increase coverage by 0.12%.
The diff coverage is 42.61%.

@@            Coverage Diff             @@
##           master    #1559      +/-   ##
==========================================
+ Coverage   36.42%   36.55%   +0.12%     
==========================================
  Files         160      163       +3     
  Lines       14900    15252     +352     
==========================================
+ Hits         5428     5576     +148     
- Misses       8882     9067     +185     
- Partials      590      609      +19
Impacted Files Coverage Δ
tests/conformance/common.go 16.34% <0.00%> (-0.13%) ⬇️
state/cockroachdb/cockroachdb.go 25.00% <25.00%> (ø)
state/cockroachdb/cockroachdb_access.go 36.59% <36.59%> (ø)
state/cockroachdb/cockroachdb_query.go 62.10% <62.10%> (ø)
nameresolution/mdns/mdns.go 70.11% <0.00%> (-0.56%) ⬇️

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 34c7eb5...fcab309. Read the comment docs.

mukundansundar
mukundansundar approved these changes Mar 22, 2022
View reviewed changes
Copy link
Contributor

@mukundansundar mukundansundar left a comment

Choose a reason for hiding this comment

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

LGTM

@mukundansundar
Copy link
Contributor

@r-c-correa Thanks for this awesome contribution. Please do make sure that the docs are accurate and follow up on that PR.

@mukundansundar
Copy link
Contributor

mukundansundar commented Mar 22, 2022 •
edited
Loading

@r-c-correa ... There needs to be a follow up PR to integrate the component into dapr ... Since it is a new component ...
Once this PR is merged, follow from point 3 .... https://github.com/dapr/components-contrib/blob/master/docs/developing-component.md#submit-your-component

You need to register the component in dapr/dapr

@r-c-correa
Copy link
Contributor Author

Of course, I do that @mukundansundar 😄
I can use that PR or need to create another?

@mukundansundar
Copy link
Contributor

Of course, I do that @mukundansundar 😄
I can use that PR or need to create another?

You would need to update components-contirb ref ... But that should suffice ....

@yaron2 yaron2 merged commit 4d44c2f into dapr:master Mar 22, 2022
@r-c-correa r-c-correa deleted the state_cockroachdb branch March 22, 2022 17:13
@berndverst berndverst added this to the v1.7 milestone Mar 29, 2022
surenderssm pushed a commit to surenderssm/components-contrib that referenced this pull request Apr 23, 2022
@r-c-correa @shubham1172
@yaron2 @Taction @berndverst @daixiang0 @surenderssm
StateStore API implementation for CockroachDB (dapr#1559)
ced9170 
* statestore for CockroachDB

Signed-off-by: Ricardo Corrêa <[email protected]>

* Upgrade nacos sdk version to meet compliance (dapr#1547)

* Upgrade nacos version

Signed-off-by: Shubham Sharma <[email protected]>

* Run go mod tidy all

Signed-off-by: Shubham Sharma <[email protected]>

* Add MaxSize to settings

Signed-off-by: Shubham Sharma <[email protected]>
Signed-off-by: Ricardo Corrêa <[email protected]>

* conformance tests for CockroachDB

Signed-off-by: Ricardo Corrêa <[email protected]>

* fix comment for internalNew func (CockroachDB)

Signed-off-by: Ricardo Corrêa <[email protected]>

* remove metadata from log (CockroachDB)

Signed-off-by: Ricardo Corrêa <[email protected]>

* removed unnecessary nil fields from GetResponse

Signed-off-by: Ricardo Corrêa <[email protected]>

* Fix ordering in transaction API for CockroachDB

Signed-off-by: Ricardo Corrêa <[email protected]>

* fix non-deterministic etag error (CockroachDB)

Signed-off-by: Ricardo Corrêa <[email protected]>

* fix error message for delete operation without key (CockroachDB)

Signed-off-by: Ricardo Corrêa <[email protected]>

Co-authored-by: Shubham Sharma <[email protected]>
Co-authored-by: Yaron Schneider <[email protected]>
Co-authored-by: Taction <[email protected]>
Co-authored-by: Bernd Verst <[email protected]>
Co-authored-by: Looong Dai <[email protected]>
Signed-off-by: Surender Singh Malik <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Taction Taction Taction left review comments

@mukundansundar mukundansundar mukundansundar approved these changes

@yaron2 yaron2 yaron2 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v1.7
Development

Successfully merging this pull request may close these issues.

StateStore API implementation for CockroachDB
7 participants
@r-c-correa @yaron2 @Taction @mukundansundar @berndverst @shubham1172 @daixiang0