-
Notifications
You must be signed in to change notification settings - Fork 2.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
Move vtorc from go-sqlite3 to modernc.org/sqlite #12214
Move vtorc from go-sqlite3 to modernc.org/sqlite #12214
Conversation
This moves vtorc from the go-sqlite3 library that uses CGO, to use modernc.org/sqlite which is a pure Go implementation. vtorc is the only component we have to build with CGO but it's causing pain for releases since we need to build it against an old Linux for linking against glibc. Using modernc.org/sqlite allows for using Go only again and makes all Vitess components buildable without CGO. In https://datastation.multiprocess.io/blog/2022-05-12-sqlite-in-go-with-and-without-cgo.html someone ran some basic benchmarks. It shows that the pure Go version can be twice as slow, but the usage of vtorc is very limited and we operate on small datasets, so I think the performance impact purely of a somewhat slower sqlite implementation is negligable. None of this is in a hot query serving path or anything like that, so I have little concern performance wise. Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com>
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
If a new flag is being introduced:
If a workflow is added or modified:
Bug fixes
Non-trivial changes
New/Existing features
Backward compatibility
|
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!
Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com>
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.
NICE! Thank you! ❤️
I was unable to backport this Pull Request to the following branches: |
The underlying issue has now been addressed in: vitessio#12214 This reverts commit 4b3b37d. Signed-off-by: Matt Lord <mattalord@gmail.com>
* Move vtorc from go-sqlite3 to modernc.org/sqlite This moves vtorc from the go-sqlite3 library that uses CGO, to use modernc.org/sqlite which is a pure Go implementation. vtorc is the only component we have to build with CGO but it's causing pain for releases since we need to build it against an old Linux for linking against glibc. Using modernc.org/sqlite allows for using Go only again and makes all Vitess components buildable without CGO. In https://datastation.multiprocess.io/blog/2022-05-12-sqlite-in-go-with-and-without-cgo.html someone ran some basic benchmarks. It shows that the pure Go version can be twice as slow, but the usage of vtorc is very limited and we operate on small datasets, so I think the performance impact purely of a somewhat slower sqlite implementation is negligable. None of this is in a hot query serving path or anything like that, so I have little concern performance wise. Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com> * Fix error handling in RowToArray Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com> --------- Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com>
Manual backport in #12223 |
* Move vtorc from go-sqlite3 to modernc.org/sqlite (#12214) * Move vtorc from go-sqlite3 to modernc.org/sqlite This moves vtorc from the go-sqlite3 library that uses CGO, to use modernc.org/sqlite which is a pure Go implementation. vtorc is the only component we have to build with CGO but it's causing pain for releases since we need to build it against an old Linux for linking against glibc. Using modernc.org/sqlite allows for using Go only again and makes all Vitess components buildable without CGO. In https://datastation.multiprocess.io/blog/2022-05-12-sqlite-in-go-with-and-without-cgo.html someone ran some basic benchmarks. It shows that the pure Go version can be twice as slow, but the usage of vtorc is very limited and we operate on small datasets, so I think the performance impact purely of a somewhat slower sqlite implementation is negligable. None of this is in a hot query serving path or anything like that, so I have little concern performance wise. Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com> * Fix error handling in RowToArray Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com> --------- Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com> * empty-commit Signed-off-by: Manan Gupta <manan@planetscale.com> * Run go mod tidy Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com> --------- Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com> Signed-off-by: Manan Gupta <manan@planetscale.com> Co-authored-by: Manan Gupta <manan@planetscale.com>
* Move vtorc from go-sqlite3 to modernc.org/sqlite (vitessio#12214) * Move vtorc from go-sqlite3 to modernc.org/sqlite This moves vtorc from the go-sqlite3 library that uses CGO, to use modernc.org/sqlite which is a pure Go implementation. vtorc is the only component we have to build with CGO but it's causing pain for releases since we need to build it against an old Linux for linking against glibc. Using modernc.org/sqlite allows for using Go only again and makes all Vitess components buildable without CGO. In https://datastation.multiprocess.io/blog/2022-05-12-sqlite-in-go-with-and-without-cgo.html someone ran some basic benchmarks. It shows that the pure Go version can be twice as slow, but the usage of vtorc is very limited and we operate on small datasets, so I think the performance impact purely of a somewhat slower sqlite implementation is negligable. None of this is in a hot query serving path or anything like that, so I have little concern performance wise. Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com> * Fix error handling in RowToArray Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com> --------- Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com> * empty-commit Signed-off-by: Manan Gupta <manan@planetscale.com> * Run go mod tidy Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com> --------- Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com> Signed-off-by: Manan Gupta <manan@planetscale.com> Co-authored-by: Manan Gupta <manan@planetscale.com>
…error (#117) * Release-15: Cherry pick vtorc no cgo (vitessio#12223) * Move vtorc from go-sqlite3 to modernc.org/sqlite (vitessio#12214) * Move vtorc from go-sqlite3 to modernc.org/sqlite This moves vtorc from the go-sqlite3 library that uses CGO, to use modernc.org/sqlite which is a pure Go implementation. vtorc is the only component we have to build with CGO but it's causing pain for releases since we need to build it against an old Linux for linking against glibc. Using modernc.org/sqlite allows for using Go only again and makes all Vitess components buildable without CGO. In https://datastation.multiprocess.io/blog/2022-05-12-sqlite-in-go-with-and-without-cgo.html someone ran some basic benchmarks. It shows that the pure Go version can be twice as slow, but the usage of vtorc is very limited and we operate on small datasets, so I think the performance impact purely of a somewhat slower sqlite implementation is negligable. None of this is in a hot query serving path or anything like that, so I have little concern performance wise. Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com> * Fix error handling in RowToArray Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com> --------- Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com> * empty-commit Signed-off-by: Manan Gupta <manan@planetscale.com> * Run go mod tidy Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com> --------- Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com> Signed-off-by: Manan Gupta <manan@planetscale.com> Co-authored-by: Manan Gupta <manan@planetscale.com> * Fix Makefile Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * Undo dir rename Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> --------- Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com> Signed-off-by: Manan Gupta <manan@planetscale.com> Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> Co-authored-by: Dirkjan Bussink <d.bussink@gmail.com> Co-authored-by: Manan Gupta <manan@planetscale.com>
* Move vtorc from go-sqlite3 to modernc.org/sqlite This moves vtorc from the go-sqlite3 library that uses CGO, to use modernc.org/sqlite which is a pure Go implementation. vtorc is the only component we have to build with CGO but it's causing pain for releases since we need to build it against an old Linux for linking against glibc. Using modernc.org/sqlite allows for using Go only again and makes all Vitess components buildable without CGO. In https://datastation.multiprocess.io/blog/2022-05-12-sqlite-in-go-with-and-without-cgo.html someone ran some basic benchmarks. It shows that the pure Go version can be twice as slow, but the usage of vtorc is very limited and we operate on small datasets, so I think the performance impact purely of a somewhat slower sqlite implementation is negligable. None of this is in a hot query serving path or anything like that, so I have little concern performance wise. Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com> * Fix error handling in RowToArray Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com> --------- Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com>
* VTOrc running PRS when database_instance empty bug fix. (vitessio#12019) * feat: convert join with database_instance to a left join and prevent fixes from running if the information from database_instance is unavailable Signed-off-by: Manan Gupta <manan@planetscale.com> * test: add tests to verify the fix works Signed-off-by: Manan Gupta <manan@planetscale.com> Signed-off-by: Manan Gupta <manan@planetscale.com> * Timeout Fixes and VTOrc Improvement (vitessio#11881) * refactor: move tests out of newfeaturestest so that they run on upgrade-downgrade tests too Signed-off-by: Manan Gupta <manan@planetscale.com> * feat: add failing ers test for handling multiple vttablet failures with default values of flags Signed-off-by: Manan Gupta <manan@planetscale.com> * feat: add a new lock-timeout flag and use that instead of remote-operation-timeout Signed-off-by: Manan Gupta <manan@planetscale.com> * feat: augment DownPrimary test to reproduce the issue of VTOrc not handling multiple failures Signed-off-by: Manan Gupta <manan@planetscale.com> * feat: remove LockShardTimeout configuration from VTOrc and add parallelism to refresh of tablets Signed-off-by: Manan Gupta <manan@planetscale.com> * log: add more logging lines around ers in vtorc Signed-off-by: Manan Gupta <manan@planetscale.com> * test: get the test to work Signed-off-by: Manan Gupta <manan@planetscale.com> * feat: fix usage of wait for replicas timeout Signed-off-by: Manan Gupta <manan@planetscale.com> * test: fix flags expected output Signed-off-by: Manan Gupta <manan@planetscale.com> * test: fix race in test now that the function is called in parallel multiple times Signed-off-by: Manan Gupta <manan@planetscale.com> * feat: fix default of onCloseTimeout to 1 second Signed-off-by: Manan Gupta <manan@planetscale.com> * test: add failing unit test to refreshTabletsInKeyspaceShard Signed-off-by: Manan Gupta <manan@planetscale.com> * feat: fix vtorc to not forget a tablet which has been deleted Signed-off-by: Manan Gupta <manan@planetscale.com> * feat: fix backward compatibility, add tests and release notes docs Signed-off-by: Manan Gupta <manan@planetscale.com> * test: fix flags output Signed-off-by: Manan Gupta <manan@planetscale.com> * test: use disable-replication-manager instead of disable-active-reparents to allow vttablets to setup replication when restarted Signed-off-by: Manan Gupta <manan@planetscale.com> * test: fix flaky test by not checking for an error Signed-off-by: Manan Gupta <manan@planetscale.com> * feat: handle the case of empty hostname in tablet initialization Signed-off-by: Manan Gupta <manan@planetscale.com> * feat: update onclose timeout to 10 seconds Signed-off-by: Manan Gupta <manan@planetscale.com> * test: fix unit test Signed-off-by: Manan Gupta <manan@planetscale.com> * feat: address review comments Signed-off-by: Manan Gupta <manan@planetscale.com> * docs: add comments explaining the test functions Signed-off-by: Manan Gupta <manan@planetscale.com> * feat: add summary docs for 'lock-shard-timeout' deprecation Signed-off-by: Manan Gupta <manan@planetscale.com> Signed-off-by: Manan Gupta <manan@planetscale.com> * log: also log error in DiscoverInstance when force discovery is specified (vitessio#11936) Signed-off-by: Manan Gupta <manan@planetscale.com> Signed-off-by: Manan Gupta <manan@planetscale.com> * VTOrc Code Cleanup - generate_base, replace cluster_name with keyspace and shard. (vitessio#12012) * feat: refactor generate commands of VTOrc to be in a single file Signed-off-by: Manan Gupta <manan@planetscale.com> * refactor: cleanup create table formatting Signed-off-by: Manan Gupta <manan@planetscale.com> * feat: cleanup the usage of IsSQLite and IsMySQL Signed-off-by: Manan Gupta <manan@planetscale.com> * feat: remove unused minimal instance Signed-off-by: Manan Gupta <manan@planetscale.com> * feat: remove unused table cluster_domain_name Signed-off-by: Manan Gupta <manan@planetscale.com> * feat: fix vtorc database to store keyspace and shard instead of cluster Signed-off-by: Manan Gupta <manan@planetscale.com> * feat: remove unused attributes Signed-off-by: Manan Gupta <manan@planetscale.com> * feat: remove unused cluster domain Signed-off-by: Manan Gupta <manan@planetscale.com> * feat: change GetClusterName to GetKeyspaceAndShardName Signed-off-by: Manan Gupta <manan@planetscale.com> * feat: fix insertion into database_instance Signed-off-by: Manan Gupta <manan@planetscale.com> * feat: fix SnapshotTopologies Signed-off-by: Manan Gupta <manan@planetscale.com> * feat: remove inject unseen primary and inject seed Signed-off-by: Manan Gupta <manan@planetscale.com> * feat: remove ClusterName from Instance Signed-off-by: Manan Gupta <manan@planetscale.com> * feat: fix Audit operations Signed-off-by: Manan Gupta <manan@planetscale.com> * feat: add Keyspace and Shard to cluster information to replace ClusterName Signed-off-by: Manan Gupta <manan@planetscale.com> * feat: fix attempt failure detection registeration Signed-off-by: Manan Gupta <manan@planetscale.com> * feat: fix blocked topology recoveries Signed-off-by: Manan Gupta <manan@planetscale.com> * feat: fix topology recovery Signed-off-by: Manan Gupta <manan@planetscale.com> * feat: reading recovery instances Signed-off-by: Manan Gupta <manan@planetscale.com> * feat: fix get replication and analysis Signed-off-by: Manan Gupta <manan@planetscale.com> * feat: fix bug in query Signed-off-by: Manan Gupta <manan@planetscale.com> * test: add tests to check that filtering by keyspace works for APIs Signed-off-by: Manan Gupta <manan@planetscale.com> * feat: remove remaining usages of ClusterName Signed-off-by: Manan Gupta <manan@planetscale.com> * refactor: fix comment explaining sleep in the test Signed-off-by: Manan Gupta <manan@planetscale.com> * feat: add code to prevent filtering just by shard and add tests for it Signed-off-by: Manan Gupta <manan@planetscale.com> Signed-off-by: Manan Gupta <manan@planetscale.com> * Fix insert query of blocked_recovery table in VTOrc (vitessio#12091) * feat: add failing test and fix the query of insertion Signed-off-by: Manan Gupta <manan@planetscale.com> * empty-commit Signed-off-by: Manan Gupta <manan@planetscale.com> Signed-off-by: Manan Gupta <manan@planetscale.com> * Fix: VTOrc forgetting old instances (vitessio#12089) * test: add a failing test for the case where the port changes for a tablet Signed-off-by: Manan Gupta <manan@planetscale.com> * feat: fix the issue by adding alias as a unique field Signed-off-by: Manan Gupta <manan@planetscale.com> * empty-commit Signed-off-by: Manan Gupta <manan@planetscale.com> Signed-off-by: Manan Gupta <manan@planetscale.com> * Move vtorc from go-sqlite3 to modernc.org/sqlite (vitessio#12214) * Move vtorc from go-sqlite3 to modernc.org/sqlite This moves vtorc from the go-sqlite3 library that uses CGO, to use modernc.org/sqlite which is a pure Go implementation. vtorc is the only component we have to build with CGO but it's causing pain for releases since we need to build it against an old Linux for linking against glibc. Using modernc.org/sqlite allows for using Go only again and makes all Vitess components buildable without CGO. In https://datastation.multiprocess.io/blog/2022-05-12-sqlite-in-go-with-and-without-cgo.html someone ran some basic benchmarks. It shows that the pure Go version can be twice as slow, but the usage of vtorc is very limited and we operate on small datasets, so I think the performance impact purely of a somewhat slower sqlite implementation is negligable. None of this is in a hot query serving path or anything like that, so I have little concern performance wise. Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com> * Fix error handling in RowToArray Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com> --------- Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com> * see if CI passes on v14.0.5 as previous release Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * Revert "see if CI passes on v14.0.5 as previous release" This reverts commit 53a0e0c. --------- Signed-off-by: Manan Gupta <manan@planetscale.com> Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com> Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> Co-authored-by: Manan Gupta <35839558+GuptaManan100@users.noreply.github.com> Co-authored-by: Dirkjan Bussink <d.bussink@gmail.com>
This moves
vtorc
from thego-sqlite3
library that uses CGO, to usemodernc.org/sqlite
which is a pure Go implementation.vtorc
is the only component we have to build withCGO
but it's causing pain for releases since we need to build it against an old Linux for linking againstglibc
.Using
modernc.org/sqlite
allows for using Go only again and makes all Vitess components buildable withoutCGO
.In
https://datastation.multiprocess.io/blog/2022-05-12-sqlite-in-go-with-and-without-cgo.html someone ran some basic benchmarks. It shows that the pure Go version can be twice as slow, but the usage of
vtorc
is very limited and we operate on small datasets, so I think the performance impact purely of a somewhat slowersqlite
implementation is negligable.None of this is in a hot query serving path or anything like that, so I have little concern performance wise.
Related Issue(s)
vtorc
since we don't needCGO
.Checklist