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

replace sqlite3 with modernc #2704

Merged
merged 3 commits into from
Apr 13, 2022
Merged

replace sqlite3 with modernc #2704

merged 3 commits into from
Apr 13, 2022

Conversation

jbreiding
Copy link
Contributor

@jbreiding jbreiding commented Apr 6, 2022

What changed?
Replaced sqlite module with pure go implementation

Why?
Towards removing CGO run temporal docker images

How did you test it?
Local unit tests and sample execution
Added memory and file variant tests for sqlite for future proofing

Potential risks
Typical risks with changing persistence dependency modules
The underlying module change does change how connection dsn is built
until schema migration becomes supported there are no implicit backwards compatibility guarantees.

Is hotfix candidate?
No

Build times

vscode ➜ /projects/temporal (master) $ go clean -modcache && go mod download && make clean-bins && time make bins
Delete old binaries...
Delete old binaries...
Build temporal-server with CGO_ENABLED=1 for linux/arm64...
go build -o temporal-server ./cmd/server
Build temporal-cassandra-tool with CGO_ENABLED=1 for linux/arm64...
go build -o temporal-cassandra-tool ./cmd/tools/cassandra
Build temporal-sql-tool with CGO_ENABLED=1 for linux/arm64...
go build -o temporal-sql-tool ./cmd/tools/sql

real    0m5.978s
user    0m7.760s
sys     0m2.080s


vscode ➜ /projects/temporal (master) $ git checkout modernc-sqlite 
Switched to branch 'modernc-sqlite'
Your branch is up to date with 'dev/modernc-sqlite'.
vscode ➜ /projects/temporal (modernc-sqlite) $ go clean -modcache && go mod download && make clean-bins && time CGO_ENABLED=0  make bins
Delete old binaries...
Delete old binaries...
Build temporal-server with CGO_ENABLED=0 for linux/arm64...
go build -o temporal-server ./cmd/server
Build temporal-cassandra-tool with CGO_ENABLED=0 for linux/arm64...
go build -o temporal-cassandra-tool ./cmd/tools/cassandra
Build temporal-sql-tool with CGO_ENABLED=0 for linux/arm64...
go build -o temporal-sql-tool ./cmd/tools/sql

real    0m3.242s
user    0m5.410s
sys     0m1.665s

Canary execution
image

image

@wxing1292
Copy link
Contributor

Plz try to run go samples / canary locally as sanity tests

@jbreiding
Copy link
Contributor Author

Plz try to run go samples / canary locally as sanity tests

I have run a few of the samples locally against this, canary is a good one to try as well.

@jbreiding jbreiding marked this pull request as ready for review April 10, 2022 02:51
@jbreiding jbreiding requested a review from a team as a code owner April 10, 2022 02:51
@jbreiding jbreiding linked an issue Apr 11, 2022 that may be closed by this pull request
@jlegrone
Copy link
Contributor

👋 really excited about this!

Heads up that Temporalite supports synchronous and journal_mode pragmas as of temporalio/temporalite-archived#30. It would be great if those could be validated against the modernc driver as well before merge.

@jbreiding
Copy link
Contributor Author

jbreiding commented Apr 11, 2022

👋 really excited about this!

Heads up that Temporalite supports synchronous and journal_mode pragmas as of DataDog/temporalite#30. It would be great if those could be validated against the modernc driver as well before merge.

Would you be willing to send a PR with a unit/integration test for this case?

We can land that test case and then rebase on top of that.

** edit below **
@jlegrone given a bit of research all the pragmas are supported that you are requesting, however they are encoded differently for modernc variant for sqlite.

Once this PR lands a change will need to happen in temporalite how the pragmas flow from cli options to dsn encoding.

More details can be found here, https://gitlab.com/cznic/sqlite/-/blob/master/all_test.go

@jlegrone
Copy link
Contributor

Would you be willing to send a PR with a unit/integration test for this case?

I don't have bandwidth at the moment, but happy to follow up after this is merged. Before communicating that the sqlite driver is production ready, we should also probably document what level of backwards compatibility we can guarantee for the driver's configuration options.

@jbreiding jbreiding merged commit b8b9eea into temporalio:master Apr 13, 2022
@jbreiding jbreiding deleted the modernc-sqlite branch April 13, 2022 22:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

using sqlite introduces CGO dependency for (only) tests
3 participants