Details

Reviewers

marcin
• jon
atul
tomek

Commits

rCOMM7081079553c2: [native/sqlite] introduce function to set up database

Summary

Create database on a fresh install (db version: 0).

Note: whenever code will face issues right now there are break or return statements but they will be handled in a different way after implementing ENG-1974

Test Plan

Perform logout user and check if

DB schema has proper structure (tables, columns, types, indexes)
journal_mode is set properly to value ’wal`

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

kamil created this revision.Oct 17 2022, 6:00 AM

kamil held this revision as a draft.

Herald added a reviewer: • jon. · View Herald TranscriptOct 17 2022, 6:00 AM

Herald added subscribers: • abosh, atul, tomek, ashoat. · View Herald Transcript

fix xommit name

kamil retitled this revision from [native/sqlite] update database setup on fresh install to [native/sqlite] introduce function to set up database.Oct 17 2022, 6:05 AM

Harbormaster completed remote builds in B12824: Diff 17609.Oct 17 2022, 6:10 AM

Harbormaster completed remote builds in B12826: Diff 17611.Oct 17 2022, 6:15 AM

kamil added a parent revision: D5378: [native/sqlite] introduce function to create database.Oct 17 2022, 6:36 AM

kamil edited the summary of this revision. (Show Details)

kamil edited the test plan for this revision. (Show Details)

kamil added a reviewer: atul.

kamil published this revision for review.Oct 17 2022, 6:43 AM

atul added 1 blocking reviewer(s): atul.Oct 19 2022, 11:16 AM

Back to your queue until D5377 is updated

This revision now requires changes to proceed.Oct 19 2022, 1:36 PM

In D5379#159648, @atul wrote:

Back to your queue until D5377 is updated

Done

atul added a reviewer: tomek.Oct 20 2022, 8:08 AM

Back to your queue until D5378 is addressed (let me know if the issues I'm seeing aren't accurate though)

This revision now requires changes to proceed.Oct 20 2022, 8:28 AM

In D5379#160340, @atul wrote:

Back to your queue until D5378 is addressed (let me know if the issues I'm seeing aren't accurate though)

D5378 adressed

marcin requested changes to this revision.Oct 24 2022, 7:17 AM

marcin added inline comments.

native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp
624 ↗	(On Diff #17611)	I would extract it. Not necessarily into separate method or function, but you could create lambda that calls rollback. I would make code nicer and more readable.
665 ↗	(On Diff #17611)	Is it intentional not to check against `set_up_database` return value? It indicates whether some SQL queries succeeded or not.

This revision now requires changes to proceed.Oct 24 2022, 7:17 AM

kamil requested review of this revision.Oct 25 2022, 9:29 AM

kamil added inline comments.

native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp
624 ↗	(On Diff #17611)	Overall I will agree but here not really, we have a different method in this file for rollback which refers to `sqlite_orm` rollback, that being said I would keep this hidden inside method only to not confuse someone in the future. But if you are sure it'll be okay and it's a good pattern I can create a lambda.
665 ↗	(On Diff #17611)	yes, thanks for pointing this out but the same answer as here: https://phab.comm.dev/D5376?id=17606#inline-35842

marcin accepted this revision.Oct 25 2022, 11:02 AM

marcin added inline comments.

native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp
624 ↗	(On Diff #17611)	Leaving the lambda thing up to you.

rebase

Harbormaster completed remote builds in B13059: Diff 17933.Oct 27 2022, 1:10 AM

Looks correct to me, but adding @tomek as blocking to get another pair of eyes on it since it's critical to get right.

rebase

Harbormaster completed remote builds in B13076: Diff 17953.Oct 27 2022, 8:16 AM

tomek accepted this revision.Oct 28 2022, 2:43 AM

tomek added inline comments.

native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp
679–682 ↗	(On Diff #17953)	It might be a good idea to add a comment explaining this case
679–694 ↗	(On Diff #17953)	We can consider making this code more concise by merging these. What do you think?

This revision is now accepted and ready to land.Oct 28 2022, 2:43 AM

simplify condition

Harbormaster completed remote builds in B13164: Diff 18068.Nov 4 2022, 2:27 AM

atul mentioned this in rCOMM5b72c8d08824: Temporary changes for staff release.Nov 8 2022, 7:27 AM

Hi @ashoat, @tomek do you think this is ready to land? (D5377, D5378, D5379). Looks like we don't have any crashes on version 153 yet (the release was patched here: https://phab.comm.dev/rCOMM5b72c8d08824c512a39d222ed9b04ce7b972f005)

I was thinking we should test for about a week. Looks like that will be reached tomorrow, so maybe we can land it then?

In D5379#166749, @ashoat wrote:

I was thinking we should test for about a week. Looks like that will be reached tomorrow, so maybe we can land it then?

Makes sense. If we discover any issues later, we could still revert the commits, but for now it looks rather safe.

Closed by commit rCOMM7081079553c2: [native/sqlite] introduce function to set up database (authored by kamil). · Explain WhyNov 14 2022, 8:40 AM

This revision was automatically updated to reflect the committed changes.

kamil added a commit: rCOMM7081079553c2: [native/sqlite] introduce function to set up database.

[native/sqlite] introduce function to set up database
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 18419

native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp

[native/sqlite] introduce function to set up databaseClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 18419

native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp

[native/sqlite] introduce function to set up database
ClosedPublic
Actions

Revision Contents
Changeset List