Rework the db version management #1775
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pm47
commented
Apr 19, 2021
Comment on lines
+82
to
+83
| * NB: we could define this method in [[fr.acinq.eclair.db.sqlite.SqliteUtils]] and [[fr.acinq.eclair.db.pg.PgUtils]] | ||
| * but it would make testing more complicated because we need to use one or the other depending on the backend. |
There was a problem hiding this comment.
This design choice is debatable but I think it's worthwhile, less duplication and simpler tests.
There was a problem hiding this comment.
Since this is a shared mechanism for all DB types, I think it's perfectly fine to have it here, especially if it makes testing easier.
eclair-core/src/main/scala/fr/acinq/eclair/db/pg/PgAuditDb.scala
Outdated
Show resolved
Hide resolved
thomash-acinq
previously approved these changes
Apr 19, 2021
|
Rebased. |
t-bast
reviewed
Apr 20, 2021
eclair-core/src/main/scala/fr/acinq/eclair/db/jdbc/JdbcUtils.scala
Outdated
Show resolved
Hide resolved
Comment on lines
+82
to
+83
| * NB: we could define this method in [[fr.acinq.eclair.db.sqlite.SqliteUtils]] and [[fr.acinq.eclair.db.pg.PgUtils]] | ||
| * but it would make testing more complicated because we need to use one or the other depending on the backend. |
There was a problem hiding this comment.
Since this is a shared mechanism for all DB types, I think it's perfectly fine to have it here, especially if it makes testing easier.
t-bast
previously approved these changes
Apr 20, 2021
(This part of the code is sensitive, but not critical, because bugs should be obvious, and their impact should be limited to the app refusing to start.) No SQL code has been modified, this is just a refactoring. The way our `getVersion`/`setVersion` was very unintuitive and led to comments like the following in tests: ```scala dbs.getVersion(statement, "network", 1) // this will set version to 1 ``` The reason is that we treat unitialized databases and up-to-date databases exactly the same way. That is why a `getVersion` will set the version to the most up-to-date if none is found. It's also why we use `CREATE IF NOT EXISTS` statements for tables and indices. I think that is very misleading and we should have a `getVersion` that only, you know, _gets_ the version (or returns `None` if no version exists). We would then treat unitialized databases and up-to-date databases differently. Also, there as some variations in the layout between files, which have been fixed. There is also less repetition.
Since we now differentiate between uninitialized and up-to-date, we don't need to make the create statements idempotent. This makes the code more strict, and we will see errors if our versioning system has bugs. Note that the `IF NOT EXISTS` statement do not bring any guarantee about the existing table (pg documentation): > A notice is issued in this case. Note that there is no guarantee that the existing relation is anything like the one that would have been created. Internal tables (used for versioning, locking, etc.) have been left unchanged.
Co-authored-by: Bastien Teinturier <[email protected]>
|
Rebased. |
t-bast
approved these changes
Apr 20, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
(This part of the code is sensitive, but not critical, because bugs
should be obvious, and their impact should be limited to the app
refusing to start.)
No SQL code has been modified, this is just a refactoring.edit: second commit does remove
IF NOT EXISTSstatements.The way our
getVersion/setVersionworked was very unintuitiveand led to comments like the following in tests:
The reason is that we treat unitialized databases and up-to-date
databases exactly the same way. That is why a
getVersionwill set theversion to the most up-to-date if none is found. It's also why we use
CREATE IF NOT EXISTSstatements for tables and indices.I think that is misleading and we should have a
getVersionthatonly, you know, gets the version (or returns
Noneif no versionexists). We would then treat unitialized databases and up-to-date
databases differently.
Also, there was some variations in the layout between files, which have
been fixed. There is also less repetition.
I'm curious to have your opinion on this @thomash-acinq, since you had to deal with
db migration recently.