-
Notifications
You must be signed in to change notification settings - Fork 1k
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
chore: Go Sql Online Store #2446
Conversation
I dont agree with the idea of rewriting online stores into Go. We will end up duplicating a lot of logic that should only live in Python in my opinion. What is the motivation behind this change? |
sdk/python/tests/integration/online_store/test_universal_online.py
Outdated
Show resolved
Hide resolved
if s.db == nil { | ||
if s.path == "" { | ||
return nil, errors.New("no database path available") | ||
} | ||
db, err := initializeConnection(s.path) | ||
s.db = db | ||
if err != nil { | ||
return nil, err | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably be guarded by a lock to prevent concurrent access creating multiple dangling connections
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added a mutex.
Adding @pyalex and @achals. Oleksii raised the concern of difficulties in writing certain tests for the Go feature server specifically and we talked about it and decided to use sqlite as a path as a simple testing backbone to do certain integration/unit testing. |
e82ef9c
to
5b5babd
Compare
Can you be more specific on the types of testing that isn't possible? Is the idea that we would reimplement all these |
I believe the idea was to have only the read path implemented for SQLite to make it easy to run a full test in pure go using a prebuilt SQLite file without crossing the boundary from python to go, or spinning up redis and writing to it before testing the go code that read it. I agree that the update method should not be included in this pr, unless I'm missing something. |
sdk/python/tests/integration/online_store/test_universal_online.py
Outdated
Show resolved
Hide resolved
cf5ef35
to
ba29126
Compare
Codecov Report
@@ Coverage Diff @@
## master #2446 +/- ##
==========================================
- Coverage 84.94% 84.92% -0.02%
==========================================
Files 127 127
Lines 10841 10841
==========================================
- Hits 9209 9207 -2
- Misses 1632 1634 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
ba29126
to
0d1d152
Compare
Signed-off-by: Kevin Zhang <[email protected]>
Signed-off-by: Kevin Zhang <[email protected]>
Signed-off-by: Kevin Zhang <[email protected]>
Signed-off-by: Kevin Zhang <[email protected]>
Signed-off-by: Kevin Zhang <[email protected]>
Signed-off-by: Kevin Zhang <[email protected]>
Signed-off-by: Kevin Zhang <[email protected]>
Signed-off-by: Kevin Zhang <[email protected]>
Signed-off-by: Kevin Zhang <[email protected]>
Signed-off-by: Kevin Zhang <[email protected]>
Signed-off-by: Kevin Zhang <[email protected]>
Signed-off-by: Kevin Zhang <[email protected]>
Signed-off-by: Kevin Zhang <[email protected]>
Signed-off-by: Kevin Zhang <[email protected]>
Signed-off-by: Kevin Zhang <[email protected]>
Signed-off-by: Kevin Zhang <[email protected]>
Signed-off-by: Kevin Zhang <[email protected]>
Signed-off-by: Kevin Zhang <[email protected]>
Signed-off-by: Kevin Zhang <[email protected]>
fdf28b2
to
941aea8
Compare
Signed-off-by: Kevin Zhang <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: achals, kevjumba The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
* Initial structure for go sqlite online store Signed-off-by: Kevin Zhang <[email protected]> * Somewhat intermediate state Signed-off-by: Kevin Zhang <[email protected]> * Add sqlite online store for go for testing Signed-off-by: Kevin Zhang <[email protected]> * Revert Signed-off-by: Kevin Zhang <[email protected]> * Revert Signed-off-by: Kevin Zhang <[email protected]> * Clean up Signed-off-by: Kevin Zhang <[email protected]> * Address review issues Signed-off-by: Kevin Zhang <[email protected]> * Fix/address issues Signed-off-by: Kevin Zhang <[email protected]> * lint Signed-off-by: Kevin Zhang <[email protected]> * Fix Signed-off-by: Kevin Zhang <[email protected]> * Fix Signed-off-by: Kevin Zhang <[email protected]> * Make integration test work Signed-off-by: Kevin Zhang <[email protected]> * Fix tests Signed-off-by: Kevin Zhang <[email protected]> * Fix tests Signed-off-by: Kevin Zhang <[email protected]> * debugging Signed-off-by: Kevin Zhang <[email protected]> * Debug Signed-off-by: Kevin Zhang <[email protected]> * Debug Signed-off-by: Kevin Zhang <[email protected]> * Debug Signed-off-by: Kevin Zhang <[email protected]> * Debug Signed-off-by: Kevin Zhang <[email protected]> * Debug Signed-off-by: Kevin Zhang <[email protected]> * Debug Signed-off-by: Kevin Zhang <[email protected]> * Remove feature_repo files Signed-off-by: Kevin Zhang <[email protected]> * update gitignore Signed-off-by: Kevin Zhang <[email protected]> * Clean up code Signed-off-by: Kevin Zhang <[email protected]> * Update go mod Signed-off-by: Kevin Zhang <[email protected]> * Update makefile Signed-off-by: Kevin Zhang <[email protected]> * Fix gitignore issue Signed-off-by: Kevin Zhang <[email protected]> * Update makefile Signed-off-by: Kevin Zhang <[email protected]> * Update makefile Signed-off-by: Kevin Zhang <[email protected]> * Update makefile Signed-off-by: Kevin Zhang <[email protected]> * Update makefile Signed-off-by: Kevin Zhang <[email protected]> * Update makefile Signed-off-by: Kevin Zhang <[email protected]> * Revert worfklow Signed-off-by: Kevin Zhang <[email protected]> * Update build path Signed-off-by: Kevin Zhang <[email protected]> * remove Signed-off-by: Kevin Zhang <[email protected]> * rename Signed-off-by: Kevin Zhang <[email protected]> * Address review Signed-off-by: Kevin Zhang <[email protected]> * fix tests Signed-off-by: Kevin Zhang <[email protected]> * Fix Signed-off-by: Kevin Zhang <[email protected]> * see if this fixes test Signed-off-by: Kevin Zhang <[email protected]> * Fix Signed-off-by: Kevin Zhang <[email protected]> * revert Signed-off-by: Kevin Zhang <[email protected]> * Will add in separate pr to update cryptography Signed-off-by: Kevin Zhang <[email protected]> Signed-off-by: joostvan <[email protected]>
What this PR does / why we need it:
Implements a basic sql online store that we can use to perform integration testing within Go instead of requiring entry point to be through Python. (This makes it quite hard to do Go specific testing). Also contains integration tests for the sql online store and the getonlinefeatures grpc service to make sure the correct data is retrieved.
Which issue(s) this PR fixes:
Fixes #