Refactor initialisation and relax tx isolation levels where possible #3901
Conversation
|
Review app for commit 0c1957f deployed to Heroku: https://hge-ci-pull-3901.herokuapp.com |
lexi-lambda
left a comment
There was a problem hiding this comment.
I’ve left one small comment, but otherwise this LGTM.
server/src-lib/Hasura/App.hs
Outdated
| schemaCache <- onNothing _icRebuildableSchemaCache | ||
| (printErrExit "expected schema cache to be initialised") |
There was a problem hiding this comment.
I would probably just pass the schema cache as a separate argument to this function rather than include it in the InitCtx wrapped in a Maybe. That way the type system can prevent this error from ever happening.
There was a problem hiding this comment.
The schema cache is now part of InitCtx{..} and is of type Maybe (RebuildableSchemaCache Run). It is is a Maybe because we get InitCtx{..} from initialiseCtx which is used in operations which do not require the schema cache to be initialised like HCExport, HCClean.
We can take schema cache outside of InitCtx{..} ofcourse but I felt it is better to be here.
Thoughts?
|
Review app for commit b95b9bb deployed to Heroku: https://hge-ci-pull-3901.herokuapp.com |
b95b9bb to
860b013
Compare
860b013 to
e7edb94
Compare
|
Deploy preview for hasura-docs ready! Built with commit 860b013 |
lexi-lambda
left a comment
There was a problem hiding this comment.
I’ve pushed a change that moves the schema cache initialization logic into mkWaiApp to avoid worrying about it at all inside initialiseCtx, since it isn’t needed until that point anyway. Let me know if my changes look okay to you.
|
Review app for commit e7edb94 deployed to Heroku: https://hge-ci-pull-3901.herokuapp.com |
Doing this earlier would run it before the catalog was migrated, causing an error.
|
Review app for commit a1509cc deployed to Heroku: https://hge-ci-pull-3901.herokuapp.com |
server/src-lib/Hasura/Server/App.hs
Outdated
| getTimeMs :: IO Int64 | ||
| getTimeMs = (round . (* 1000)) `fmap` getPOSIXTime | ||
|
|
||
| initialiseSchemaCache :: m (E.PlanCache, SchemaCacheRef, Maybe UTCTime) |
There was a problem hiding this comment.
Can we rename this to reflect that this also migrates catalog (if required)? Or split the catalog migration into a different top-level function?
The reason being at some point (in near future) we might want to run a check if catalog migration is required (using a different txiso level) and then skip it (that will solve #660)
There was a problem hiding this comment.
Renamed to migrateAndInitialiseSchemaCache.
|
Review app for commit be2c6fb deployed to Heroku: https://hge-ci-pull-3901.herokuapp.com |
|
Review app https://hge-ci-pull-3901.herokuapp.com is deleted |
…asura#3901) Co-authored-by: Alexis King <lexi.lambda@gmail.com>
Description
Currently, we are using
Serializabletx isolation levels for all system processes. This is not required as most (one exception is catalog migration) of the system processes are either single statement read or write i.e (no repeated reads). Reducing the tx isolation level of such processes is generally good.Affected components
Related Issues
#660