-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
manage schema cache when horizontally scaled (closes #1182) #1574
Conversation
Review app for commit 428f70d deployed to Heroku: https://hge-ci-pull-1574.herokuapp.com |
We should add tests for this? Will require some interesting black magic from @shahidhk @arvi3411301 I think ;) |
-> UserInfo -> SchemaCache -> HTTP.Manager | ||
-> RQLQuery -> m (BL.ByteString, SchemaCache) | ||
runQuery pool isoL userInfo sc hMgr query = do | ||
runQuery pool isoL serverId userInfo sc hMgr query = do |
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.
Should return (BL.ByteString, Maybe (SchemaCache, Timestamp)
. Just (SC, TS)
implies that the schemacache has to be replaced. After this we don't have to use queryNeedsReload
in the locations where runQuery
is being called.
server/src-rsr/initialise.sql
Outdated
@@ -403,3 +403,32 @@ CREATE TABLE hdb_catalog.remote_schemas ( | |||
definition JSON, | |||
comment TEXT | |||
); | |||
|
|||
CREATE TABLE hdb_catalog.hdb_cache_update_event ( |
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.
We are recording schema updates. So let's call it hdb_schema_update_event
(cascade the changes through out the PR).
-- Postgres notification handler | ||
notifyHandler notif = do | ||
let eventChannel = PGChannel $ bsToTxt $ PQ.notifyRelname notif | ||
when (eventChannel == pgChannel) $ |
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.
Why do this check again?
onError e = do | ||
logError logger threadType $ TEQueryError e | ||
-- Push a listen failed event to queue | ||
STM.atomically $ STM.writeTQueue eventsQueue CUEListenFail |
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.
Don't emit a 'listen error' event. Emit an event once the connection is re-established after an error. You'll probably need to expose this functionality from pg-client-hs, probably listen
can take something like this as an argument:
data NotifyHandler
= NotifyHandler
{ _ehOnInit :: IO ()
, _ehOnMessage :: Notify -> IO ()
}
cacheUpdateEventProcessor pool logger httpManager eventsQueue | ||
cacheRef lk serverId cacheInit = do | ||
-- Initiate previous event IO reference | ||
prevEventRef <- IORef.newIORef Nothing |
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.
You don't have to store the previous event ref after the above suggested change (emit event on reconnection).
Resolve Conflicts: server/src-exec/Main.hs server/src-lib/Hasura/Server/Init.hs
Resolve Conflicts: server/src-exec/Main.hs server/src-exec/Ops.hs server/src-lib/Hasura/Server/App.hs server/src-lib/Hasura/Server/Init.hs server/src-lib/Hasura/Server/Query.hs server/src-rsr/migrate_from_9_to_10.sql
Review app for commit 933b734 deployed to Heroku: https://hge-ci-pull-1574.herokuapp.com |
Review app for commit e9c7c2b deployed to Heroku: https://hge-ci-pull-1574.herokuapp.com |
Review app for commit 1632dd1 deployed to Heroku: https://hge-ci-pull-1574.herokuapp.com |
Review app for commit bf30119 deployed to Heroku: https://hge-ci-pull-1574.herokuapp.com |
…l-engine into issue-1182-cache-update
Review app for commit 054b26a deployed to Heroku: https://hge-ci-pull-1574.herokuapp.com |
Review app for commit 4472a21 deployed to Heroku: https://hge-ci-pull-1574.herokuapp.com |
Review app for commit 6338341 deployed to Heroku: https://hge-ci-pull-1574.herokuapp.com |
Review app for commit 4a74839 deployed to Heroku: https://hge-ci-pull-1574.herokuapp.com |
Review app for commit 524bf91 deployed to Heroku: https://hge-ci-pull-1574.herokuapp.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.
LGTM
Review app https://hge-ci-pull-1574.herokuapp.com is deleted |
Description
What component does this PR affect?
Requires changes from other components? If yes, please mark the components:
Related Issue
close #1182
Solution and Design
pg-client-hs
has been updated to expose a function that helps listen topostgres
notifications over achannel
in this PRhdb_catalog.hdb_cache_update_event
whenever any/v1/query
(that changes metadata) is requested. A trigger notifies acache update
event viahasura_cache_update
channellistener
andprocessor
. Thelistener
thread listens to events onhasura_cache_update
channel and pushed into aQueue
. Theprocessor
thread fetches events from thatQueue
and processes it. Thus server rebuilds schema cache from database and updates.Type
Checklist: